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

src: remove Atomics.notify alias #22830

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Sep 13, 2018

The version of V8 we are shipping has the alias built in, so we can remove this.

cc @nodejs/v8 @rwaldron @leobalter

Closes #21219

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@devsnek devsnek added semver-major PRs that contain breaking changes and should be released in the next major version. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 13, 2018
@targos targos closed this Sep 13, 2018
@targos targos reopened this Sep 13, 2018
@targos
Copy link
Member

targos commented Sep 13, 2018

Is this semver-major because wake is removed?

@devsnek
Copy link
Member Author

devsnek commented Sep 13, 2018

@targos that's a good point... wake is still there. I'm not sure if that's intended or not.

@devsnek
Copy link
Member Author

devsnek commented Sep 13, 2018

@devsnek devsnek mentioned this pull request Sep 13, 2018
3 tasks
@@ -5,48 +5,4 @@
(function(global) {
// https://github.com/nodejs/node/issues/14909
if (global.Intl) delete global.Intl.v8BreakIterator;

Copy link
Member

Choose a reason for hiding this comment

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

👆 Should probably check in on the global.Intl bit in a follow-up (not this PR) to see if it's still needed too.

@leobalter
Copy link
Contributor

the changes LGTM.

I wonder if Node could still warn about using Atomics.wake - if it's still available on V8 - so we avoid usage. This doesn't seem to be the responsibility of Node, thou, but it would nice to advice using Atomics.notify instead.

@jdalton
Copy link
Member

jdalton commented Sep 13, 2018

@leobalter Related #22844.

@leobalter
Copy link
Contributor

Just repeating my statement here, I prefer the approach from #22844 and I hope it supersedes this PR.

That path should be easier to define a semver release as well, as things would be stated very similar to the user.

@devsnek
Copy link
Member Author

devsnek commented Sep 14, 2018

I'm actually really conflicted about which path to take here. On the one hand, Atomics.wake has been removed from the spec. On the other, there's been a signal from chrome that it might keep wake and this might possibly mean that wake probably ends up in annex b.

In either case, we do actively have a warning being emitted saying that Atomics.wake will be removed, so node as a platform has some autonomy from v8/chakra/whatever.

Thoughts?

@devsnek
Copy link
Member Author

devsnek commented Sep 18, 2018

Since no one has any thoughts i'm closing this in favor of #22844

@devsnek devsnek closed this Sep 18, 2018
@devsnek devsnek deleted the remove-atomics-notify-alias branch September 18, 2018 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants