search

defunctzombie / node-url

node.js core url module as a module
MIT License
375 stars 96 forks source link

Add fileURLToPath #59

Open mischnic opened 2 years ago

mischnic commented 2 years ago

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

ljharb commented 2 years ago

This seems great. Can you confirm that this implementation/tests match every version of node that has this function?

codecov-commenter commented 2 years ago

Codecov Report

Merging #59 (5a3f3e4) into master (b449442) will increase coverage by 0.16%. The diff coverage is 100.00%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
url.js 97.16% <100.00%> (+0.16%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b449442...5a3f3e4. Read the comment docs.

mischnic commented 2 years ago

Can you confirm that this implementation/tests match every version of node that has this function?

The function was added in Node 10.12.0. All of the fileURLToPath-related tests I added pass with Node 10.24.1, 12.22.7, 14.18.2, 16.13.1, 17.2.0. (By changing the test script to var url = require('url'); and running it with that version)

JumpLink commented 1 year ago

Is there any chance that this will be merged?

ljharb commented 1 year ago

@JumpLink yes, or else it'd be closed.

starwiz-7 commented 9 months ago

I've been using a package which uses fileURLToPath internally for a bunch of things inside webpack 5 and using this package to polyfill url. I have been receving build errors for this.

Any chance we can merge this implementation?

Cavdy commented 8 months ago

yeah I have same experience as @starwiz-7, would be great to have this merged

cascornelissen commented 5 months ago

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?

niksy commented 2 months ago

If you’re looking for solution where you can alias url to userland implementation (e.g. bundler alias option), node-stdlib-browser (note: I’m the author of the module) adds additional exports to the ones provided by node-url.

ljharb commented 2 months ago

@niksy this package is that solution; if yours has functionality this one is missing, it’d be great to PR that in.

niksy commented 2 months ago

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 URL implementation that works all the way down to Node 0.4 and it isn’t big of a dependancy?

ljharb commented 2 months ago

Yep, that’s right.