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

Revert "vm: support parsing a script in a specific context" #16136

Closed

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Oct 11, 2017

This reverts commit 7d95dc3.
It causes breakages in the Browserify test suite

I want to land this in less than 48 hours as it is blocking the release of 8.7.0

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8.x vm Issues and PRs related to the vm subsystem. labels Oct 11, 2017
@addaleax
Copy link
Member

addaleax commented Oct 11, 2017

This seems like a combination of bugs in browserify’s dependencies and its test suite – nothing that you would run into when you’re just using browserify for its purpose? I don’t think the PR in question is semver-major and it doesn’t look to me like we’d need to revert this.

(edit: Maybe it’s obvious from the above but I also don’t think we should hold up 8.7.0 over this…)

This reverts commit 7d95dc3.
It causes breakages in the Browserify test suite
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Oct 11, 2017

@addaleax the failures come from a buggy Traceur polyfill for object.assign

#14888 (comment)

My concern is that while this may not technically be Semver-Major it will potentially break things in the ecosystem that were working in 8.6.0

Open to discussion on this if people are all on the same page. Would like to see browserify's test suite fixed as well, but perhaps we can keep this commit and prioritize that instead

@addaleax
Copy link
Member

Yeah, but the failures also appear only in a new vm context as far as I can tell (or otherwise they would already be happening anyway?), which is the bit that’s specific to browserify’s test suite and that will not happen when the actual browserified code is run in a, well, browser…?

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 because I am still not convinced a buggy test in a basically under-maintained test suite is worth reverting a Core change. (Note: Browserify still works!)

If that is not convincing enough of a reason, one can just cache Object.assign at the beginning of the file, and call the saved function. That is purely an ad-hoc fix, especially since the rest of the code base uses Object.assign() with no problem, but if that makes @MylesBorins happier I'm okay with it.

@fhinkel
Copy link
Member

fhinkel commented Oct 12, 2017

Not in favor of reverting.

@jasnell
Copy link
Member

jasnell commented Oct 12, 2017

I'm also -1 on reverting

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Oct 12, 2017

Closing revert... Will work on getting browserify test suite fixed

@MylesBorins MylesBorins deleted the revert-7d95dc385c branch November 14, 2017 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants