-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Absolute Windows paths are handled as invalid URL path in ESM import() #34765
Comments
cc @nodejs/modules |
This behavior isn't restricted to windows paths, the same is true for absolute Unix paths as well. The module import specifiers only support URLs, not file paths. To import file paths, they need to be converted to URLs first on all platforms (like you mentioned). The general problem with supporting both URLs and file paths is that some URLs are also valid (but different!) file paths and vice versa. We decided to go with URLs over file paths to be consistent with other runtimes (e.g. browsers) that are also using URL-based import specifiers. |
Do you mean that |
@StarpTech Yes,
Not to mention that host-relative URLs are host-relative. Which means that even the "valid" cases that only use alphanumerical characters with slashes (the characters that happen to be interpreted the same in posix paths and // Works!
import 'file:///mnt/c/Users/starptech/test.mjs';
// Fails, sometimes!
import '/mnt/c/Users/starptech/test.mjs'; The second import would fail if the module above was loaded from a |
If it can't be guaranteed, from a design perspective the API is prone to errors. Why we don't enforce the |
@StarpTech i don't understand what you want to enforce, can you clarify? right now https://url.spec.whatwg.org/#relative-url-string is what is being followed to https://url.spec.whatwg.org/#path-absolute-url-string . |
One other option we can consider here would be to improve the error message in the case of an absolute windows path. |
The problem here is that I had absolutely not clue on why some code I wrote was not working on windows as the code was working smoothly on Linux and Mac. The developer experience for this was far from great, because the only way to debug this would be to have a Windows machine
I think this would be the bare minimum change that improve the developer experience. |
We could also just make the paths not work on Linux or Mac. That seems odd though and would remove some URL support. |
I think improving the error message so it is clear is a good first step. If I am mistaken it seems like the issue is manually creating a URL... is the exact same code working on OSX and failing on windows? |
@bmeck forget it. I think logging a warning message in case of an absolute URL without a special scheme e.g
@MylesBorins Yes, it works by the use of |
What I think would be helpful is adding a check in the ERR_UNSUPPORTED_ESM_URL_SCHEME constructor which checks if the url's scheme is a single letter and the system is on windows, and then offering that you probably want to prefix it with |
/cc @weswigham |
Refs: #34765 PR-URL: #34795 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Refs: #34765 PR-URL: #34795 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Refs: #34765 PR-URL: #34795 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Refs: nodejs#34765 PR-URL: nodejs#34795 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Refs: #34765 PR-URL: #34795 Backport-PR-URL: #35385 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
I believe this is resolved since #34795 landed |
thank you so much |
related issue nodejs/node#34765
related issue nodejs/node#34765
* chore: add windows ci test * fix: dynamic import file path in Windows * fix: try forward slashes * chore: log out file path * fix: pathToFileURL().href related issue nodejs/node#34765 * chore: log out error * fix: try adding cwd() * fix: cross platform write file script related shelljs/shx#192 * fix: try next-video/process import * fix: cross platform glob expansion * chore: use codecov v4 * chore: move next.config.js mock test file
Related: #31710
What steps will reproduce the bug?
Windows paths are interpreted as invalid file URL. In Linux and Mac, the issue can't be reproduced.
Fail
Works
Root cause
#31710 (comment)
What is the expected behavior?
Interpret windows paths as valid URL paths so we can provide consistent cross-platform behaviour.
What do you see instead?
@mcollina
The text was updated successfully, but these errors were encountered: