-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update deps #1230
Update deps #1230
Conversation
LGTM 👍 @jmm? |
This all sounds good. Having to pin a dep like punycode because it's pinned in another dep seems a little wonky though. Then it'll get out of sync if url updates it and no one thinks to update it here. npm doesn't provide a more elegant way to accomodate that situation, eh? Re: the new version number, I have a bit of mixed feelings about that, since it probably wouldn't break 99.9% of things, but ultimately I guess I'd err on the side of caution / predictability and bump the major unless this stuff can wait until some other things that would necessitate that anyway get rolled in. I think there's some urgency for you to get this to fix stuff in watchify though, right @zertosh? At least it would be a super easy changelog to digest. BTW, FWIW I'm still operating on the guidelines I was pointed to here that "Declaring formal releases remains the prerogative of the project maintainer." -- therefore not for me to say :) @terinjokes were you in favor of just staying at v9? |
@jmm With the process changes, I'll be in favor of a v10 (since it seems like it could break things subtly. And true, I believe I've only published patch releases before. EDIT
The multi-stage install that npm@3 brings should dedupe a bit more than the current install. I'll have to do some tests to see if it would figure this one out. |
@terinjokes Yeah, sorry, I just got to that one (reading the whole thing in a bit) and saw that it might be a moot point.
I figured that, at least as far as it pertains to me at this point in time, that meant any release, even patch level. What would you say that means by "formal release" -- major only? major.minor? everything? |
@jmm Not urgent, just a convenience. But since the
O_O eep. I don't know. Is formal = major? Or does this mean that publishing rights to npm may or may not be given a contributor?
@terinjokes When that time comes we should totally revisit this. We should be mindful of changes to |
Unless we have clarification, probably best to allow substack to tag the releases then. |
Makes sense to wait then - don't want to overstep bounds w/o clarification. |
I don't mean to screw up anyone else's workflow -- I assumed that people who've been collaborators here longer than me might have different parameters for how to contribute. (I think I was the most recent addition until you, @zertosh.) Or maybe you're on to something here:
Maybe being designated an npm maintainer is implied thumbs up for cutting releases?
I was thinking about that too, but I was wondering if it'll also dedupe on |
Having thought about this over the day, I'm not sure I like pinning to punycode here, since it creates a maintainability issue for us to remember to upgrade it when we upgrade url. @zertosh this is to save a few megabytes on install, correct? |
@terinjokes correct. Also, it's not like EDIT: #funfact |
@terinjokes I'm starting to think that. hughsk's policy over at envify is "In the future I'd say submit a PR, and if you don't hear from me after a day then merge away :)" hughsk/envify#23 (comment). And he gave me the same collaborator agreement. |
@zertosh I've felt comfortable merging in about the same time period. It's cutting releases that I'm not sure of. |
All of this looks great, you don't need my permission to cut a release! Just remember to add a quick description to the changelog. |
With that out of the way, to the issue at hand. I continue to prefer not pinning punycode, even if it does mean two get installed until user switches over to npm@3. As an aside, I'm not even sure why punycode is listed as a dependency since recent node versions will never resolve to it (but that's an issue for that repo). Ignoring the questionable change to process.nextTick, do we have any other PRs pending (or about to be pending) that would require the bump? |
@terinjokes I think that's it for now. I went ahead and made punycode ^1.3.2. I'll merge this in and #1231. You can do the honors of cutting 10.0 😄 |
@zertosh ok, happening now. |
^
already covers the version, so that over in watchify I can count on Fix mysteriously disappearing "expose" and "entry" module-deps#80 always being in whatever is the next browserify version we publish.The motivation for the deduping is #707 (comment).
@jmm and @terinjokes: This should be published as 9.0.9. But since #1222 hasn't been published, strict semver dictates that we move to 10.0.0. I think that's over kill for removing the "v" flag. I'll hold off publishing until you chime in.