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

move per-context changes from c++ to js #21518

Closed
wants to merge 2 commits into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jun 25, 2018

move the changes made to new contexts from c++ to js.

second commit adds a warning when Atomics.wake is used and changes Atomics.notify.name to notify

/cc @rwaldron

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

@devsnek devsnek requested a review from addaleax June 25, 2018 07:17
@devsnek devsnek requested a review from hashseed June 25, 2018 07:17
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 25, 2018
@devsnek devsnek changed the title Refactor/per context things move per-context changes from c++ to js Jun 25, 2018
@devsnek devsnek force-pushed the refactor/per-context-things branch 2 times, most recently from 907720a to 2f8157a Compare June 25, 2018 07:41
} else {
global.console.error(`Atomics: ${warning}`);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure, is there any reason why we wouldn’t want to do a normal deprecation cycle for these (i.e. make this PR semver-major)?

Copy link
Member Author

@devsnek devsnek Jun 25, 2018

Choose a reason for hiding this comment

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

I wasn't quite sure where to document it and like @rwaldon said it's not very widely used. I'm ok with removing the message though

Copy link
Member Author

Choose a reason for hiding this comment

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

should this still be considered semver major?

@devsnek devsnek force-pushed the refactor/per-context-things branch from 2f8157a to e681bea Compare June 27, 2018 19:03
@devsnek
Copy link
Member Author

devsnek commented Jun 27, 2018

@devsnek
Copy link
Member Author

devsnek commented Jun 27, 2018

landed in 759809f...dcb371f

@devsnek devsnek closed this Jun 27, 2018
@devsnek devsnek deleted the refactor/per-context-things branch June 27, 2018 22:15
devsnek added a commit that referenced this pull request Jun 27, 2018
PR-URL: #21518
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
devsnek added a commit that referenced this pull request Jun 27, 2018
PR-URL: #21518
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jun 28, 2018
PR-URL: #21518
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jun 28, 2018
PR-URL: #21518
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos
Copy link
Member

targos commented Jun 28, 2018

@devsnek Be careful with the commit messages. per_context is not a subsystem.

@devsnek
Copy link
Member Author

devsnek commented Jun 28, 2018

@targos i assumed once it was created it became a subsystem. what would the correct label have been?

@addaleax
Copy link
Member

@devsnek As I understand it, subsystems for files under lib/ generally refer to public modules, and the rest would generally be considered lib?

The point of subsystems is that they make reading changelogs easier, because we group commits by them; if it’s not something that immediately affects users of Node.js but still touches the source files, lib or src would be appropriate imo

@targos
Copy link
Member

targos commented Jun 28, 2018

@addaleax Thanks, that's along the lines of what I was writing :)

@refack
Copy link
Contributor

refack commented Jun 29, 2018

@devsnek could you reopen this PR. I want to see the what Jenkins reported.
NM

@Trott
Copy link
Member

Trott commented Jun 29, 2018

It does appear that this landed with a red CI. I know it's sometimes painful to slog it out until you get to green or yellow, but I'd ask that we really work hard to do that. I'm bisecting right now because something broke the --without-intl build (which is what failed in the CI in this PR)...

@Trott
Copy link
Member

Trott commented Jun 29, 2018

d13cdd9 broke the build. So the red CI in this case wasn't an infrastructure issue but was in fact a test failure resulting from the change set.

@Trott
Copy link
Member

Trott commented Jun 29, 2018

PR to revert the one bad commit: #21587

@Trott
Copy link
Member

Trott commented Jun 29, 2018

Narrower alternative fix suggested by @devsnek and @refack in #21589

1 similar comment
@Trott
Copy link
Member

Trott commented Jun 29, 2018

Narrower alternative fix suggested by @devsnek and @refack in #21589

@targos targos mentioned this pull request Jul 3, 2018
targos added a commit that referenced this pull request Jul 3, 2018
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
targos added a commit that referenced this pull request Jul 4, 2018
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
targos added a commit that referenced this pull request Jul 4, 2018
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
alexeykuzmin added a commit to electron/node that referenced this pull request Sep 3, 2018
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants