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.

Sign up for GitHub

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

Jump to bottom

lib: move web global bootstrapping to the expected file #47881

Closed
wants to merge 2 commits into from
Closed

lib: move web global bootstrapping to the expected file #47881

wants to merge 2 commits into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 5, 2023 •
edited
Loading

Moves the definition of queueMicrotask and structuredClone to internal/bootstrap/web/exposed-window-or-worker.js as they are defined in HTML WindowOrWorkerGlobalScope. This also suggests they can be disabled with the option --no-browser--globals.

Also, remove the obsolete deletion bufferBinding.zeroFill.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 5, 2023
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51706/

@github-actions github-actions bot mentioned this pull request May 10, 2023
Open
25 tasks
jasnell
jasnell approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Could this be considered semver-major?

@legendecas
Copy link
Member Author

@jasnell: Could this be considered semver-major?

This doesn't affect users that don't utilize --no-browser-globals. AFAICT, electron is the primary user of the flag: https://github.com/electron/electron/blob/13e309e1fb2491af7c4bfd6bc6e786dbeb8dfc4b/shell/common/node_bindings.cc#L551. So yeah, this can be semver-major.

@legendecas legendecas added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 10, 2023
This was referenced May 11, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51761/

This was referenced May 13, 2023
Open
Open
Open
@aduh95
Copy link
Contributor

aduh95 commented May 17, 2023 •
edited
Loading

Could this be considered semver-major?

This doesn't affect users that don't utilize --no-browser-globals. AFAICT, electron is the primary user of the flag: https://github.com/electron/electron/blob/13e309e1fb2491af7c4bfd6bc6e786dbeb8dfc4b/shell/common/node_bindings.cc#L551. So yeah, this can be semver-major.

I would argue that it's a fix, not a breaking change. In all likeliness, folks using --no-browser-globals would not want Node.js to define queueMicrotask nor structuredClone.

@legendecas
Copy link
Member Author

legendecas commented May 19, 2023 •
edited
Loading

I was under the impression that changes to the global are semver-major. Unfortunately, I didn't find a written rule for it. Regarding the semantic of the flag --no-browser-globals, I'd be fine for this to be categorized as a fix.

@codebytere would you mind helping out here to see if un-exposing the queueMicrotask and structuredClone on the global would break electron? Thank you!

@legendecas legendecas added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue Add this label to land a pull request using GitHub Actions. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in f3b713d...ee1b6ab

nodejs-github-bot pushed a commit that referenced this pull request Jun 15, 2023
@legendecas @nodejs-github-bot
lib: move web global bootstrapping to the expected file
77df1d8 
PR-URL: #47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
nodejs-github-bot pushed a commit that referenced this pull request Jun 15, 2023
@legendecas @nodejs-github-bot
lib: remove obsolete deletion of bufferBinding.zeroFill
ee1b6ab 
PR-URL: #47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@legendecas legendecas deleted the realm/globals branch June 15, 2023 06:36
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@legendecas @RafaelGSS
lib: move web global bootstrapping to the expected file
562b3d4 
PR-URL: #47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@legendecas @RafaelGSS
lib: remove obsolete deletion of bufferBinding.zeroFill
b6ae411 
PR-URL: #47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@legendecas @Ceres6
lib: move web global bootstrapping to the expected file
6142dc9 
PR-URL: nodejs#47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@legendecas @Ceres6
lib: remove obsolete deletion of bufferBinding.zeroFill
f1cdad5 
PR-URL: nodejs#47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@legendecas @Ceres6
lib: move web global bootstrapping to the expected file
0d58487 
PR-URL: nodejs#47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@legendecas @Ceres6
lib: remove obsolete deletion of bufferBinding.zeroFill
dac0341 
PR-URL: nodejs#47881
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 8, 2023
@ruyadorno
Copy link
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it to land in v18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@aduh95 aduh95 aduh95 approved these changes

@JungMinu JungMinu JungMinu approved these changes

@XadillaX XadillaX XadillaX approved these changes

@anonrig anonrig anonrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@legendecas @nodejs-github-bot @aduh95 @ruyadorno @jasnell @anonrig @XadillaX @joyeecheung @JungMinu