-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
fix remote-path normalization (#127) #128
Conversation
@@ -9,7 +9,6 @@ var cloneable = require('cloneable-readable'); | |||
var replaceExt = require('replace-ext'); | |||
var cloneStats = require('clone-stats'); | |||
var cloneBuffer = require('clone-buffer'); | |||
var removeTrailingSep = require('remove-trailing-separator'); |
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.
moved to normalize to allow remote paths with trailing slash
return str; | ||
} | ||
|
||
if (urlRegex().test(str) && !/^www\./.test(str)) { |
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.
url-regex
matches on strings like www.google.de which are also a valid directory.
So let path.normalize
do the normalization
"replace-ext": "^1.0.0" | ||
"replace-ext": "^1.0.0", | ||
"normalize-url": "1.6.1", | ||
"url-regex": "3.2.0" |
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.
Fix versions for normalize-url
and url-regex
as the latest version only support node >= 4.
Could be updated when vinyl
drops node 0.10...0.12 support
Didn't look yet, but we should have tests for:
Will review later today |
Definitely needs all of the tests stated by @contra I'd also like to wait on feedback from @darsain @jonschlinkert and @doowb |
Hmm, since path handling in vinyl is completely geared towards file system paths, maybe this is an opportunity to rethink the URL handling so that it's more idiomatic for URLs. For example:
Maybe we should add support for edit: idea... we could just create conventions for how the URL segments created by edit edit: regardless of the approach, it seems like the code in the pr doesn't make sense for this lib. If we're going to go to these measures for normalization, maybe it's better to allow a |
This feels like an entrance to a very deep and complicated rabbit hole that will lead to a lot of edge cases and weird behavior as we try to unify 2 different resource pointing concepts into one ambiguous monster. For start, how would we handle search and hash parts of the url? or ports, or I think it would be simpler to create a separate "virtual online resource format" project, differentiate between them with |
It worked fine prior to path normalization being added - vinyl-http, vinyl-ftp, etc. are all things that exist (now broken due to normalization addition) Worst case we can revert the normalization and do it a layer up |
yeah, this sums up what I was thinking as well. |
@contra it was a major bump because it changed path semantics |
That's why i asked if the dropped support for remote paths was the intended behaviour and vinyl should focus on file system paths. What was the the goal in introducing normalization? What's the advantage in making this lib more strictly focused on filesystems?
I personally like the idea. This would address all concerns regarding this pr and let's the user opt-in to the opinionated normalization process which in my case would be totally fine to fix my issue. Maybe i am in the wrong position to make decisions here but i would be fine to propose another PR for the normalization overloading ;) |
If the user is passing in a normalize function then why wouldn't they normalize the path before passing it in? Not a fan of that. I don't see why it would be a huge deal to mandate people normalize before passing it in (previous behavior, better docs) and leave it out of the base level library. What was the normalization fixing? IIRC every adapter does it's own normalization. |
I completely disagree. However, I've actually thought of an elegant way to handle both. I'll write up my thoughts in the morning. |
I actually believe this problem can and should be solved by my idea of an Enhanced Stat Object (#105). Usage would look something like: var file = new Vinyl({
path: 'http://google.com',
stat: {
isRemote: true
}
}) The |
@phated yup, that'd be a pretty good solution |
Closing this PR because it won't make it into master. Thanks for the discussion/feedback. I would go for a) "drop the normalization + enhance documentation" or b) something like #130. But no matter what option you'll choose. I'm sure it will work out ;) |
No description provided.