-
Notifications
You must be signed in to change notification settings - Fork 45
feat: add escapedName to parsed Result #18
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
Conversation
|
It seems that builds have been failing or erring for Node 0.8 for quite some time, so please don't hold that against this PR. 😃 |
| var assert = require("assert") | ||
| var util = require("util") | ||
| var semver = require("semver") | ||
| var path = require("path") |
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.
To avoid confusion this should be in a separate commit (or PR; I think it may be time to standardize this code base).
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.
Ok, good point, I will undo this.
|
The CLI team is dropping support for 0.8 as of now, so I'll probably pull that from |
escapedName uri-encodes the slash for scoped names otherwise escapedName matches name the idea is that anywhere you would use name when talking to a registry, you should now use escapedName instead
5576b36 to
b8d31ba
Compare
|
@othiym23 Attempted to address your concerns. Tests pass for me locally. Let me know if you see anything else. |
|
Rebased and landed as e92fcc9. Thanks, Andrew! |
escapedNameURI-encodes the slash for scoped names such that@foo/barbecomes@foo%2fbar. For unscoped names,escapedNameshould just matchname, either null or otherwise.The idea is that anywhere you would use
namewhen talking to a registry, you should now useescapedNameinstead. This should eliminate the need for duplicating lines like this across multiple repos/packages/modules that need to parse a package name when working with a registry, which is something we do quite often among open and closed source code at npm. This little package seems an ideal place to centralize this logic.When implementing this, I considered a few different options:
Using
Object.definePropertyto addescapedNameas a getter method that was always derived fromname, conditionally escaping based onscopeThis kept things simple in terms of not needing to "follow" all the places in the logic where
nameandscopeare set. Its drawback, however, was a hidden value when relying onObject.prototype.toString(), such as here, which resulted in output like:Which is obviously less than ideal.
Using a wrapper or decorator function to populate
escapedNamewheneverresis returned.This fixed the
toString()problem, but since thenpa()function can actually return in a few different places, I ended up "following" the return statements rather than wherenameandscopeare actually set. This seemed less than ideal since the it didn't make the relationship between the derived property and the original properties obvious.So instead I opted to align the setting of
escapedNamein close proximity to the setting ofscopeandnamein an explicit, procedural way.