Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serve the index.html for SPA routing FIXING #1231

Closed
wants to merge 9 commits into from

Conversation

moataz2002
Copy link
Contributor

@moataz2002 moataz2002 commented Mar 30, 2024

Adding Feature of: Single Page Application (SPA) routing support

Related Issues:

#1230

Description:

This PR adds support for Single Page Application (SPA) routing to the Neutralinojs server. When a client requests a non-existing resource, the server will now serve the index.html file instead, allowing the client-side routing of the SPA to handle the requested route.

Changes Made:

Modified the handleHTTP function in neuserver.cpp to check for non-existing resources and serve the index.html file accordingly.

Modified the execCommand Serve the index.html for SPA routing & checking if the requested file is not found and if it's not an API request

Added logic to serve index.html if the requested resource is not found and it's not an API request in
Testing:

Tested the changes locally by running a Neutralinojs application with React Router and confirmed that client-side routing works as expected.

Verified that regular resource requests and API requests are still handled correctly.

Additional Notes:

Ensure that the documentRoot setting is correctly configured in the application settings to point to the root directory of the SPA.

Copy link

@zoroxide zoroxide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same idea xD

api/os/os.cpp Outdated
Comment on lines 260 to 266
// Check if the requested file is not found and if it's not an API request
if (commandResult.exitCode == 404 && !commandResult.stdOut.empty() && commandResult.stdOut.find("API_REQUEST_MARKER") == std::string::npos) {
// Serve the index.html for SPA routing
string indexFilePath = getPath("config") + "/index.html"; // Adjust the path as needed
commandResult = os::execCommand("cat " + indexFilePath, "", background, cwd);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain why this section is needed and what is API_REQUEST_MARKER. Can we add your solution without modifying the os::execCommand function? Thanks 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified: Integrate the solution to serve the index.html for SPA routing directly after the execCommand call

Comment on lines 215 to 219
string indexPath = documentRoot + "/index.html"; // Adjust the path as needed
ifstream file(indexPath);
stringstream buffer;
buffer << file.rdbuf();
con->set_body(buffer.str());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use fs::readFile() instead of using C++ standard library directly? 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified function uses fs::exists() to check for file existence and fs::readFile() to read the file content if the file exists.

Copy link
Member

@shalithasuranga shalithasuranga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @moataz2002, Thanks so much for your pull request. Could you please check code change comments? We can merge this once we resolve those points 🎉

…s::readFile() to read the file content if the file exists.
@moataz2002
Copy link
Contributor Author

Hello @moataz2002, Thanks so much for your pull request. Could you please check code change comments? We can merge this once we resolve those points 🎉

Hello @shalithasuranga , I have resolved the code comments points 😇

@shalithasuranga
Copy link
Member

Hello @moataz2002, thanks so much for your work. I've implemented this with a bit different strategy via 5f9778a

Thanks 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants