Skip to content

Fixes #355: add absolute path checking to util.join(); #356

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

Closed
wants to merge 1 commit into from

Conversation

mutoo
Copy link

@mutoo mutoo commented Sep 4, 2018

No description provided.

@coveralls
Copy link

coveralls commented Sep 4, 2018

Pull Request Test Coverage Report for Build 502

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.898%

Totals Coverage Status
Change from base Build 498: 0.0%
Covered Lines: 910
Relevant Lines: 1053

💛 - Coveralls

@tromey
Copy link
Contributor

tromey commented Sep 4, 2018

I tend to think this isn't the way to go. The source map spec is worded in terms of URLs, not OS fiile names. So, special-casing Windows file names seems odd.

I wonder if maybe something in your webpack pipeline ought to be using file: URLs. Or maybe it is (I can't tell) and I'm mistaken -- more information might help.

@mutoo
Copy link
Author

mutoo commented Sep 5, 2018

@tromey Yeah, I agree with you! The sources should be URLs since they're used by the browser. In our pipeline, the sources are sitting in the local filesystem, so that it'd prefix with file: protocol. Maybe the webpack-internal:: protocol is better.

@mutoo mutoo changed the title Fixes #355 by adding test for windows device path; Fixes #355: add absolute path checking to util.join(); Sep 5, 2018
@mutoo
Copy link
Author

mutoo commented Sep 5, 2018

I removed the test for windows device path in util.isAbsolute() function.
But the util.join() still needs the isAbsolute() to test the absolute path, so I'll keep it.

@loganfsmyth
Copy link
Contributor

I've landed #371 which may resolve this. Would you be able to check on master? It's not clear to me what this patch is looking to resolve.

@mutoo
Copy link
Author

mutoo commented Nov 16, 2018

@loganfsmyth I've reviewed the code on master, the bug was fixed.
The issue I tried to fix was that the old join function didn't check an absolute path correctly.
This can be close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants