Skip to content

lib: stop installing fetch if no_browser_globals is true#41969

Merged
nodejs-github-bot merged 1 commit intonodejs:masterfrom
RaisinTen:lib/stop-installing-fetch-if-no_browser_globals-is-true
Feb 16, 2022
Merged

lib: stop installing fetch if no_browser_globals is true#41969
nodejs-github-bot merged 1 commit intonodejs:masterfrom
RaisinTen:lib/stop-installing-fetch-if-no_browser_globals-is-true

Conversation

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen 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-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 14, 2022
@RaisinTen RaisinTen 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

function setupWebCrypto() {
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)
@Mesteery Mesteery mentioned this pull request Feb 14, 2022
Merged
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@github-actions github-actions bot 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-actions github-actions bot 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/

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

nodejs-github-bot commented Feb 16, 2022

Landed in a82c1a1

@github-actions github-actions bot mentioned this pull request Feb 17, 2022
Open
28 tasks
@RaisinTen RaisinTen 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-actions github-actions bot 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]>
@bengl bengl 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panva panva panva approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@targos targos targos approved these changes

+2 more reviewers

@Linkgoron Linkgoron Linkgoron approved these changes

@Mesteery Mesteery Mesteery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

Fetch should not be installed if no_browser_globals==true

10 participants

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

Comments