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: stop installing fetch if no_browser_globals is true #41969

Merged
merged 1 commit into from
Feb 16, 2022
Merged

lib: stop installing fetch if no_browser_globals is true #41969

merged 1 commit into from
Feb 16, 2022

Conversation

RaisinTen
Copy link
Contributor

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

@nodejs-github-bot
Copy link
Collaborator

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

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

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

@Mesteery Mesteery Mesteery approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Linkgoron Linkgoron Linkgoron approved these changes

@panva panva panva approved these changes

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