Skip to content
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 process to ~0.11.0 #1231

Merged
merged 2 commits into from Apr 30, 2015
Merged

Update process to ~0.11.0 #1231

merged 2 commits into from Apr 30, 2015

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Apr 27, 2015

Fixes #1179. This also solves my major-version-bump dilemma from #1230 because this merits it for sure.

The reason for having to pass in clearTimeout into the context in tests is because there is now a queue in process.nextTick that depends on it (see defunctzombie/node-process#38 and defunctzombie/node-process@v0.10.1...v0.11.0.).

cc: @jmm @terinjokes

@zertosh
Copy link
Member Author

zertosh commented Apr 27, 2015

cc: @slorber

@terinjokes
Copy link
Contributor

Glad to get the Q issue resolved. I'm happy to merge both these in and do a major release. @jmm?

@zertosh
Copy link
Member Author

zertosh commented Apr 27, 2015

@terinjokes Cool! I merged browserify/insert-module-globals#43 and bumped it here in 064f5a1 - just for consistency, doesn't really affect anything in browserify.

@slorber
Copy link

slorber commented Apr 27, 2015

nice is there a browserify release yet?

@terinjokes
Copy link
Contributor

@slorber not yet ✨

@jmm
Copy link
Collaborator

jmm commented Apr 27, 2015

@zertosh @terinjokes You guys probably understand this issue better than I do, but I looked through a bunch of the related issues / PRs and it LGTM.

This also solves my major-version-bump dilemma from #1230 because this merits it for sure.

Does this break BC somehow? I got the impression that stuff that's already working would continue working with this change. Or would the changes in defunctzombie/node-process/pull/38 actually break something people were relying on before? The other stuff I saw in recent commits there didn't seem like it would be breaking. Sorry if I'm being dense here.

Aside: it sounds like maybe the issue with Q in particular has been addressed independently there -- not that it has any bearing on this PR.

@zertosh
Copy link
Member Author

zertosh commented Apr 27, 2015

@jmm I would think this is a BC because it fundamentally changes how process.nextTick works by enforcing a queue that doesn't die on errors (plus, now it depends on clearTimeout, which broke our tests). Granted, it is fixing a bug, but a bug that might've been relied on. Since this is such an obscure feature - does that kinda fall into the semver gray area? I don't know :( I could really use some guidance right now.

@jmm
Copy link
Collaborator

jmm commented Apr 27, 2015

@zertosh I'm not an expert on semver, so it does seem like a gray area to me. If you think people may have relied on the previous behavior then I think it would make sense to bump the major version. I wonder what node-process would've done with the version if it had already been >= 1.

@zertosh zertosh mentioned this pull request Apr 30, 2015
zertosh added a commit that referenced this pull request Apr 30, 2015
@zertosh zertosh merged commit a615650 into browserify:master Apr 30, 2015
@zertosh zertosh deleted the update-process branch April 30, 2015 01:03
@terinjokes
Copy link
Contributor

Reading more into the issue for the changelog, I'll find it really strange if anyone did rely on the behavior, since it prevented any future process.nextTick tasks from running.

@zertosh
Copy link
Member Author

zertosh commented Apr 30, 2015

@terinjokes Maybe? I'm by no means a semver expert 😛

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

Successfully merging this pull request may close these issues.

Q stops working with Browserify >= 8.1.0 due to nextTick changes
4 participants