-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add fileURLToPath #59
base: master
Are you sure you want to change the base?
Conversation
db4bb68
to
cf23d04
Compare
This seems great. Can you confirm that this implementation/tests match every version of node that has this function? |
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 97.00% 97.16% +0.16%
==========================================
Files 1 1
Lines 367 388 +21
Branches 133 140 +7
==========================================
+ Hits 356 377 +21
Misses 11 11
Continue to review full report at Codecov.
|
test/index.js
Outdated
var fromURL = url.fileURLToPath(new URL(fileURLToPathTestCase.fileURL)); | ||
assert.strictEqual(fromURL, fileURLToPathTestCase.path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test case needs to be skipped when typeof URL !== 'function'
, for older nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is a problem with the implementation as well: https://github.com/mischnic/node-url/blob/5a3f3e445f71d688aa945ebd778034bccf8fb4e2/url.js#L794-L795
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it is; we can’t add anything that depends on URL without a full polyfill for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using url.parse
instead might be problematic because that behaves differently to the WHATWG one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to use url.parse
instead now. Only protocol, pathname and hostname are used. So I hope his is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that particular polyfill doesn't work on old enough engines to be viable. We'd need to locate one that works down to node 0.4 and equivalent browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think https://github.com/zloirock/core-js/blob/master/packages/core-js/web/url.js (which imports https://github.com/zloirock/core-js/blob/master/packages/core-js/modules/web.url.js) should be compatible enough?
It appears to work with Node 0.10.48
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! That implies it's possible. We'd need to build our own implementation tho, based on that one is fine, since that's too large a single package to depend on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So even using require("core-js-pure/web/url")
would be too large to install/load?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there's a ton of reasons I don't want that particular dependency.
Co-authored-by: Jordan Harband <[email protected]>
The function was added in Node 10.12.0. All of the |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
I've been using a package which uses Any chance we can merge this implementation? |
yeah I have same experience as @starwiz-7, would be great to have this merged |
I don't want to add to the pile of +1's, but we are in a similar situation. I'm willing to help out, is there anything we can do to help get this merged and released? |
If you’re looking for solution where you can alias |
@niksy this package is that solution; if yours has functionality this one is missing, it’d be great to PR that in. |
Absolutely! I’ve done the changes from my side since at the time it was easier to just patch it, but I would definitely like it if it’s done upstream. From what I can see in this PR, everything is implemented, but from the comments I see that major blocker is |
Yep, that’s right. |
This adds
fileURLToPath
.In Node, this uses the current platform's file path format (windows vs posix). Since https://github.com/browserify/path-browserify also only implements posix, I think it makes sense to do the same here.
I've copied the implementations and the majority of the test cases from Node itself. I think this makes sense to ensure compatibility, but I'm not sure if this poses problems regarding the license