-
Notifications
You must be signed in to change notification settings - Fork 37
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
Make file: URLs (mostly) RFC 8909 compliant #60
Conversation
Arguably beneficial side effect of this change: |
92132b3
to
97d2923
Compare
res.saveSpec = 'file:' + path.relative(where, res.fetchSpec) | ||
// always put the '/' on where when resolving urls, or else | ||
// file:foo from /path/to/bar goes to /path/to/foo, when we want | ||
// it to be /path/to/foo/bar |
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.
if using file:foo
from /path/to/bar
, wouldn't we want it to be /path/to/bar/foo
instead?
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, that's what the comment is saying.
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.
hahaha no, it says /path/to/foo/bar
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.
oh, haha, yeah, swapped the parts in the comment. The code is right, though. I'll fix 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.
LGTM 👍
The change made in e8c4301 (#53) was reasonable, but had the unfortunate side effect of breaking backwards compatibility for urls like 'file://.'. Further investigation showed that we really weren't complying with RFC 8909's specifications regarding file: URLs, and even further investigation than that showed that it'll be a breaking change if we do. This patch isolates the non-8909 compliance bits into a single block that can be disabled with an environment variable and easily removed when we are willing to take on that breaking change. The bug fixed by the commit that broke file://. is still fixed, and we obey RFC8909 otherwise. PR-URL: #60 Credit: @isaacs Close: #60 Reviewed-by: @ruyadorno
97d2923
to
8a6d887
Compare
Relates to: npm#60 (comment)
The change made in e8c4301 was
reasonable, but had the unfortunate side effect of breaking backwards
compatibility for urls like 'file://.'.
Further investigation showed that we really weren't complying with RFC
8909's specifications regarding file: URLs, and even further
investigation than that showed that it'll be a breaking change if we do.
This patch isolates the non-8909 compliance bits into a single block
that can be disabled with an environment variable and easily removed
when we are willing to take on that breaking change. The bug fixed by
the commit that broke file://. is still fixed, and we obey RFC8909
otherwise.
References