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

fix: normalize API routes using Rust path entities #32

Merged
merged 10 commits into from
Nov 18, 2022

Conversation

Angelmmiguel
Copy link
Contributor

@Angelmmiguel Angelmmiguel commented Nov 17, 2022

In the current main version, file paths are not normalized properlies as APIs. This is causing inconsistent behaviors in different environments:

  • In unix, the API path includes the root folder the CLI receives as argument. For example, for wws ./examples/js-basic, the URL of the handler.js should be /handler. Note that ./examples/js-basic is the local folder to read the files, not a prefix for the final URL. However, the HTTP endpoint it generates is `/examples/js-basic/handler.
  • In windows, it just doesn't work. The handler.js is not accessible (See [1]).

This is mainly caused by converting file paths to string and apply transformations later. Since paths may include special components such as . and ./, applying operations like replace can have different outcomes depending on the environment. This also causes inconsistent behaviors when a final backslash is added or not to the CLI argument.

In this PR, I changed that implementation to run all required transformations directly on the path. Instead of replacing and subtracting from a converted string, I iterate over the components to remove any special entity and keep only the Normal ones. Finally, I convert them to a String to be used as the API path.

I tested the current changes on both Windows and Mac (Unix) and they work great:

win_working

[1]
win_err
win_err2

It closes #30

@Angelmmiguel Angelmmiguel added the 🐛 bug Something isn't working label Nov 17, 2022
@Angelmmiguel Angelmmiguel self-assigned this Nov 17, 2022
@Angelmmiguel
Copy link
Contributor Author

I found an error when testing subfolders in Windows. It's using the \ separator and that's causing actix not to define the route properly.

Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Looking great, lots of tests! A couple of comments :)

.github/workflows/build.yml Outdated Show resolved Hide resolved
src/router.rs Outdated Show resolved Hide resolved
src/router.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Angelmmiguel! It looks great! Just a small optional comment about the GH workflow setup

.github/workflows/build.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@assambar assambar left a comment

Choose a reason for hiding this comment

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

Ship it!

@Angelmmiguel Angelmmiguel merged commit 27fb996 into main Nov 18, 2022
@ereslibre ereslibre deleted the 30-fix-prefixed-routes branch November 18, 2022 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix wrong routes when passing a path as an argument
4 participants