fix: replace url-join with path.posix.join
#334
Conversation
sodatea
requested review from
alexander-akait and
michael-ciniawsky
as code owners
|
|
Codecov Report
@@ Coverage Diff @@
## master #334 +/- ##
=======================================
Coverage 96.82% 96.82%
=======================================
Files 7 7
Lines 252 252
=======================================
Hits 244 244
Misses 8 8
Continue to review full report at Codecov.
|
sodatea
changed the title
url-join with path.join, fixes #333url-join with path.posix.join, fixes #333
michael-ciniawsky
changed the title
url-join with path.posix.join, fixes #333url-join with path.posix.join
|
@sodatea Happy to see it go an use a core module instead, but windows test are currently failing with this patch https://ci.appveyor.com/project/sokra/webpack-dev-middleware/build/1.0.110/job/fxenmru3b8ujigbt |
| @@ -278,41 +284,41 @@ describe('GetFilenameFromUrl', () => { | |||
| url: '/test/windows.txt', | |||
| outputPath: 'c:\\foo', | |||
| publicPath: '/test', | |||
| // this is weird, but it's legal-ish, and what URI parsing produces | |||
| expected: 'c://\\foo/windows.txt' | |||
There was a problem hiding this comment.
We need ro make sure this is valid to be removed, windows paths recognize both separators to my knowledge and this therefore will fail for some folks having / in their paths for whatever reason :) Can you elaborate on why this can be removed please ?
There was a problem hiding this comment.
Where we might expect
c:\foo\barwe instead get a URL-izedc://\foo/bar. Either works with the default path translation on Windows, though technically it's wrong.
If I understand correctly, the // is not indended to be preserved but now that it still works we didn’t bother removing it.
There was a problem hiding this comment.
Could you try with path.normalize and leaving the expected values ?
const filename = path.posix.join(path.normalize(...), ...)There was a problem hiding this comment.
After refactoring those Windows-specific tests still passes (path.posix.join(path.normalize('C:\\foo'), 'windows.txt') returns C:\\foo/windows.txt)
But a lot of other tests failed for unknown reason. So I suppose my initial implementation is legit.
There was a problem hiding this comment.
ping @michael-ciniawsky
There was a problem hiding this comment.
Paths are not URLs there must be a reason why url-join was used here instead. I didn't write this code so I'm not sure if 'this only handles file paths' renders 💯 true. I think the issue with path.nomalize was, that we would need to use the path.posix variant instead
- const filename = path.posix.join(path.normalize(...), ...)
+ const filename = path.posix.join(path.posix.normalize(...), ...)But leave it for now until we know how to proceed here in general :)
Codecov Report
@@ Coverage Diff @@
## master #334 +/- ##
=======================================
Coverage 96.82% 96.82%
=======================================
Files 7 7
Lines 252 252
=======================================
Hits 244 244
Misses 8 8
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #334 +/- ##
==========================================
- Coverage 96.82% 96.81% -0.02%
==========================================
Files 7 7
Lines 252 251 -1
==========================================
- Hits 244 243 -1
Misses 8 8
Continue to review full report at Codecov.
|
| @@ -1,7 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| const path = require('path'); | |||
There was a problem hiding this comment.
const { join } = require('path')
There was a problem hiding this comment.
Looks good let's try it, I still wonder why url-join was used here in the first place...
@sodatea Thx
lib/middleware.js
Outdated
| @@ -1,7 +1,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| const { posix: { join } } = require('path'); | |||
There was a problem hiding this comment.
Please just use const path = require('path' ) ... path.posix.join(...) like you did in lib/util.js
lib/util.js
Outdated
| @@ -1,10 +1,10 @@ | |||
| 'use strict'; | |||
|
|
|||
| const path = require('path'); | |||
| const { parse } = require('url'); | |||
| const querystring = require('querystring'); | |||
| const pathabs = require('path-is-absolute'); | |||
There was a problem hiding this comment.
This should be gone by now #335. Could you rebase with the PR with latest master to ensure we won't get a bad merge here please?
There was a problem hiding this comment.
Doesn't seemed to be the case the line is still there (?)
There was a problem hiding this comment.
Ahh, sorry, forgot to push -f 😅
Should be fixed now.
sodatea
dismissed stale reviews from michael-ciniawsky and alexander-akait
via
8e08fce
|
@sodatea Since this removes a dependency could you update the lockfile please? Otherwise this is ready to land |
michael-ciniawsky
added
status: approved
and removed
pr: review
priority: 3 (important)
severity: 3 (broken)
labels
Type
Issues
SemVer