fix: support system paths (windows)

[relevantsam]

With bun on windows arriving, the static plugin for Elysia needs some minor fixes where the assumption was made that the url path separator would match the system separator.

Problem: When using alwaysStatic, servers run on windows with this plugin have invalid paths generated and added to the router, such as: /public/images\sample.png.

Background: The plugin at initialization time when either in production with sufficiently few static files or when alwaysStatic is set to true uses readdir to find the names of files to serve. The names are the full paths for the files, and are returned using the operating system’s native separator (on windows this is \. This has cascading effects through the plugin when run on windows, because the plugin assumes a *NIX style /, which was fine until bun released windows support.

Solution: Identified areas of the code reliant on / and added compatibility for windows’ and any other future native path separators.

Tested by running suite on windows + exploring with the example.

[touhidurrr]

I am not sure why do we need to import sep to access filenames. Because in windows, both forward and backward slash is supported to access paths. This is also explained in node path docs. So, I don't see where this is required. I am also using staticPlugin on windows with no problems. So, this pr feels unnecessary.

Can you explain exactly what do we need to fix? Maybe explain a case where staticPlugin does not work on windows. Because from your description, I am not being able to understand this.

[relevantsam]

I am not sure why do we need to import sep to access filenames. Because in windows, both forward and backward slash is supported to access paths. This is also explained in node path docs. So, I don't see where this is required. I am also using staticPlugin on windows with no problems. So, this pr feels unnecessary.

Can you explain exactly what do we need to fix? Maybe explain a case where staticPlugin does not work on windows. Because from your description, I am not being able to understand this.

Thanks for the questions. I’m not sure if you’ve tried running the tests in this repo on windows natively but they do not pass as is because of the issue I’ve outlined - I believe you are not using the alwaysStatic flag which is why it might be working for you (though it is liable to break if you run in with the node production environment flag on windows). I updated the description for additional clarity - I felt the problem was self-evident but I can see that I could’ve been clearer.

[touhidurrr]

The plugin at initialization time when either in production with sufficiently few static files or when alwaysStatic is set to true uses readdir to find the names of files to serve.

if that is the case, then why not simply use name = name.replace(/\\/g, '/') to resolve this issue. Does this work?

This will convert windows style paths to unix style paths which we can then use everywhere. Because, as I said before, windows supports posix style paths also. This is better for simplicity. And users will not need to import sep to pass paths to the plugin.

[relevantsam]

The plugin at initialization time when either in production with sufficiently few static files or when alwaysStatic is set to true uses readdir to find the names of files to serve.

if that is the case, then why not simply use name = name.replace(/\\/g, '/') to resolve this issue. Does this work?

This will convert windows style paths to unix style paths which we can then use everywhere. Because, as I said before, windows supports posix style paths also. This is better for simplicity. And users will not need to import sep to pass paths to the plugin.

Why do you think users will need to “import sep to pass paths to the plugin”? Users can directly set the paths as appropriate for their platform if they want. I used sep so that the tests will work on whatever platform they are run on.

Using the node / bun standard path module, which again, is already used by this plugin which does explicitly this in a forward compatible and common way is simpler, prevents unnecessary regex parsing / string manipulation when the sep does match the uri separator and is a simpler, better practice than including manual regex and applying it to every path.

[touhidurrr]

I used sep so that the tests will work on whatever platform they are run on.

That is my point, it should not be required to use sep to get this behavior. It should work by default.

[relevantsam]

I used sep so that the tests will work on whatever platform they are run on.

That is my point, it should not be required to use sep to get this behavior. It should work by default.

I’d like a maintainer to direct the preferred behavior here. We could, for example, assume the user will always pass uri style separators for the ignored files attribute. Or we could try to account for this and swap uri style separators for the separator provided by sep on every provided path where sep != the uri style separator. But then we are doing extra work (only at init time potentially) for each string on those platforms.

Currently, since they are passing file paths, I have treated the given paths as system local paths.

Since this is an api decision I will defer to @SaltyAom / another maintainer to make a call.

[touhidurrr]

I would like mention that most tools / files that we use to specify ignore paths on windows (such as .gitignore, .prettierignore etc.) all allow users to use posix style paths on windows and it works fine. Pretty sure they support both windows and posix style path but most developers use posix style paths anyways, and it works on both windows and linux anyways. So, i think it will only confuse users if this plugin has some other kind of behavior.

Since windows also support posix paths to begin with, I would have just replaced \ with / during readdir() or when importing ignorepaths if it was me since the rest would have worked right by default. (I myself wrote a file server with nodejs before and never faced any problems with it using posix paths on windows).

Anyways, that is my whole take here. It is up to maintainers to decide what to do but I wanted to share my experience and analysis regarding this matter anyways for the purpose of a good discussion (more eyes are always better for any coding related issues).

[relevantsam]

I used sep so that the tests will work on whatever platform they are run on.

That is my point, it should not be required to use sep to get this behavior. It should work by default.

I’d like a maintainer to direct the preferred behavior here. We could, for example, assume the user will always pass uri style separators for the ignored files attribute. Or we could try to account for this and swap uri style separators for the separator provided by sep on every provided path where sep != the uri style separator. But then we are doing extra work (only at init time potentially) for each string on those platforms.

Currently, since they are passing file paths, I have treated the given paths as system local paths.

Since this is an api decision I will defer to @SaltyAom / another maintainer to make a call.

I would like mention that most tools / files that we use to specify ignore paths on windows (such as .gitignore, .prettierignore etc.) all allow users to use posix style paths on windows and it works fine. Pretty sure they support both windows and posix style path but most developers use posix style paths anyways, and it works on both windows and linux anyways. So, i think it will only confuse users if this plugin has some other kind of behavior.

Since windows also support posix paths to begin with, I would have just replaced \ with / during readdir() or when importing ignorepaths if it was me since the rest would have worked right by default. (I myself wrote a file server with nodejs before and never faced any problems with it using posix paths on windows).

Anyways, that is my whole take here. It is up to maintainers to decide what to do but I wanted to share my experience and analysis regarding this matter anyways for the purpose of a good discussion (more eyes are always better for any coding related issues).

It’s an interesting improvement. I wouldn’t call it a coding related issue since it’s a modification to how the api works. Right now this PR enables the plugin to work at all on windows. I recommend you make your own PR off of this one to add this functionality you are interested in 🙂