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

Fetch should not be installed if no_browser_globals==true #41816

Closed
targos opened this issue Feb 1, 2022 · 4 comments · Fixed by #41969
Closed

Fetch should not be installed if no_browser_globals==true #41816

targos opened this issue Feb 1, 2022 · 4 comments · Fixed by #41969
Labels
fetch Issues and PRs related to the Fetch API

Comments

@targos
Copy link
Member

targos commented Feb 1, 2022

See

node/src/node.cc

Lines 366 to 373 in 7904331

if (!no_browser_globals()) {
result = ExecuteBootstrapper(
this, "internal/bootstrap/browser", &node_params, &node_args);
if (result.IsEmpty()) {
return MaybeLocal<Value>();
}
}

I could not add fetch to bootstrap/browser.js because it's not allowed to access CLI flags from this file.
@targos targos added the fetch Issues and PRs related to the Fetch API label Feb 1, 2022
RaisinTen added a commit to RaisinTen/node that referenced this issue Feb 13, 2022
@RaisinTen
src: add fetch to bootstrap/browser.js
2ece79a 
Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen mentioned this issue Feb 13, 2022
Closed
@RaisinTen
Copy link
Contributor

PR: #41958

@RaisinTen
Copy link
Contributor

Labeling this as blocked, as there's probably no way to add fetch to the bootstrap script and honor the CLI option at the same time. We should probably just wait for fetch to become stable to be able to add it to bootstrap/browser.js. Feel free to remove the label if anyone thinks otherwise.

@RaisinTen RaisinTen added the blocked PRs that are blocked by other issues or PRs. label Feb 14, 2022
@targos targos removed the blocked PRs that are blocked by other issues or PRs. label Feb 14, 2022
@targos
Copy link
Member Author

targos commented Feb 14, 2022

as there's probably no way to add fetch to the bootstrap script and honor the CLI option at the same time

I agree with that, but there's probably a way to expose no_browser_globals() to JS and add it to the condition that installs fetch.

RaisinTen added a commit to RaisinTen/node that referenced this issue Feb 14, 2022
@RaisinTen
lib: stop installing fetch if no_browser_globals is true
6f7bae1 
Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen mentioned this issue Feb 14, 2022
Merged
@RaisinTen
Copy link
Contributor

Ah, then #41969 should be okay, right?

nodejs-github-bot pushed a commit that referenced this issue Feb 16, 2022
@RaisinTen
lib: stop installing fetch if no_browser_globals is true
a82c1a1 
Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
@RaisinTen @bengl
lib: stop installing fetch if no_browser_globals is true
bd4e550 
Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
@RaisinTen @bengl
lib: stop installing fetch if no_browser_globals is true
1f1198d 
Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
bengl pushed a commit that referenced this issue Feb 21, 2022
@RaisinTen @bengl
lib: stop installing fetch if no_browser_globals is true
6d8fb59 
Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
bengl pushed a commit that referenced this issue Feb 21, 2022
@RaisinTen @bengl
lib: stop installing fetch if no_browser_globals is true
8c912ab 
Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
bengl pushed a commit that referenced this issue Feb 22, 2022
@RaisinTen @bengl
lib: stop installing fetch if no_browser_globals is true
68ec8d5 
Fixes: #41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #41969
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fetch Issues and PRs related to the Fetch API
Projects
None yet
Milestone
No milestone
2 participants
@targos @RaisinTen