Skip to content

lib: stop installing fetch if no_browser_globals is true#41969

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
Merged
nodejs-github-bot merged 1 commit into from
Feb 16, 2022
Merged

lib: stop installing fetch if no_browser_globals is true#41969

nodejs-github-bot merged 1 commit into from
Feb 16, 2022

Conversation

@RaisinTen
Copy link
Member

@RaisinTenRaisinTen commented Feb 14, 2022

Fixes: #41816
Signed-off-by: Darshan Sen [email protected]

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 14, 2022

Review requested:

  • @nodejs/startup

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 14, 2022
@RaisinTenRaisinTen mentioned this pull request Feb 14, 2022
Closed
@aduh95
Copy link
Contributor

aduh95 commented Feb 14, 2022

I think if a user use --experimental-fetch, they do want the global fetch. If #41811 lands, then it would make sense to make the default value of --experimental-fetch equal to no_browser_globals, but until then I don't think we should restrict it more.

@targos
Copy link
Member

targos commented Feb 14, 2022

@aduh95 I think no_browser_globals is here because there were issues with Electron and globals that exist both in Node.js and Chromium. If a user has --experimental-fetch in their NODE_OPTIONS, it could still be a problem.

@aduh95
Copy link
Contributor

aduh95 commented Feb 14, 2022

If we're doing this, the same check should be added for

node/lib/internal/bootstrap/pre_execution.js

Lines 169 to 172 in 849991c

functionsetupWebCrypto(){
if(!getOptionValue('--experimental-global-webcrypto')){
return;
}

Mesteery added a commit to Mesteery/node that referenced this pull request Feb 14, 2022
@Mesteery
lib: stop installing webcrypto if no_browser_globals is true
15bcb4c
Refs: nodejs#41969 (comment)
@MesteeryMesteery mentioned this pull request Feb 14, 2022
Merged
@panvapanva added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsbot mentioned this pull request Feb 15, 2022
Open
20 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2022

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

@RaisinTenRaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2022
@RaisinTenRaisinTen mentioned this pull request Feb 15, 2022
Closed
@github-actionsgithub-actionsbot mentioned this pull request Feb 16, 2022
Open
33 tasks
@MesteeryMesteery added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 16, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 16, 2022
@nodejs-github-botnodejs-github-bot merged commit a82c1a1 into nodejs:masterFeb 16, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 16, 2022

Landed in a82c1a1

@github-actionsgithub-actionsbot mentioned this pull request Feb 17, 2022
Open
28 tasks
@RaisinTenRaisinTen deleted the lib/stop-installing-fetch-if-no_browser_globals-is-true branch February 17, 2022 04:49
nodejs-github-bot pushed a commit that referenced this pull request Feb 17, 2022
@Mesteery
lib: stop installing webcrypto if no_browser_globals is true
fdab137
Refs: #41969 (comment) PR-URL: #41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Feb 18, 2022
Open
26 tasks
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@RaisinTen@bengl
lib: stop installing fetch if no_browser_globals is true
bd4e550
Fixes: nodejs#41816 Signed-off-by: Darshan Sen <[email protected]> PR-URL: nodejs#41969 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@Mesteery@bengl
lib: stop installing webcrypto if no_browser_globals is true
2ab877f
Refs: nodejs#41969 (comment) PR-URL: nodejs#41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@RaisinTen@bengl
lib: stop installing fetch if no_browser_globals is true
1f1198d
Fixes: nodejs#41816 Signed-off-by: Darshan Sen <[email protected]> PR-URL: nodejs#41969 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
bengl pushed a commit that referenced this pull request Feb 21, 2022
@RaisinTen@bengl
lib: stop installing fetch if no_browser_globals is true
6d8fb59
Fixes: #41816 Signed-off-by: Darshan Sen <[email protected]> PR-URL: #41969 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
bengl pushed a commit that referenced this pull request Feb 21, 2022
@Mesteery@bengl
lib: stop installing webcrypto if no_browser_globals is true
9c783c3
Refs: #41969 (comment) PR-URL: #41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
@benglbengl mentioned this pull request Feb 21, 2022
Merged
bengl pushed a commit that referenced this pull request Feb 21, 2022
@RaisinTen@bengl
lib: stop installing fetch if no_browser_globals is true
8c912ab
Fixes: #41816 Signed-off-by: Darshan Sen <[email protected]> PR-URL: #41969 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
bengl pushed a commit that referenced this pull request Feb 21, 2022
@Mesteery@bengl
lib: stop installing webcrypto if no_browser_globals is true
53e2e8c
Refs: #41969 (comment) PR-URL: #41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
bengl pushed a commit that referenced this pull request Feb 22, 2022
@RaisinTen@bengl
lib: stop installing fetch if no_browser_globals is true
68ec8d5
Fixes: #41816 Signed-off-by: Darshan Sen <[email protected]> PR-URL: #41969 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
bengl pushed a commit that referenced this pull request Feb 22, 2022
@Mesteery@bengl
lib: stop installing webcrypto if no_browser_globals is true
c3b0e72
Refs: #41969 (comment) PR-URL: #41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
@danielleadams

This comment was marked as resolved.

Sign in to view
@aduh95
Copy link
Contributor

aduh95 commented Apr 20, 2022

no_browser_globals is not available on v16.x because #40532 never landed there.

danielleadams pushed a commit that referenced this pull request Apr 21, 2022
@Mesteery@danielleadams
lib: stop installing webcrypto if no_browser_globals is true
6898161
Refs: #41969 (comment) PR-URL: #41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
@Mesteery@danielleadams
lib: stop installing webcrypto if no_browser_globals is true
e80043f
Refs: #41969 (comment) PR-URL: #41971 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Nitzan Uziely <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panvapanvapanva approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@lpincalpincalpinca approved these changes

@targostargostargos approved these changes

+2 more reviewers

@LinkgoronLinkgoronLinkgoron approved these changes

@MesteeryMesteeryMesteery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fetch should not be installed if no_browser_globals==true

10 participants

@RaisinTen@nodejs-github-bot@aduh95@targos@danielleadams@panva@Linkgoron@benjamingr@lpinca@Mesteery