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.

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

src: add fetch to bootstrap/browser.js #41958

Closed

Conversation

RaisinTen
Copy link
Contributor

Fixes: #41816
Signed-off-by: Darshan Sen raisinten@gmail.com

Fixes: nodejs#41816
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 13, 2022
@RaisinTen
Copy link
Contributor Author

Hmm, seems to fail during the node_mksnapshot step:

node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:40:19)
    at node:internal/bootstrap/browser:79:5

#
# Fatal error in v8::ToLocalChecked
# Empty MaybeLocal.
#

and the exception is coming from

"Should not query options before bootstrapping is done");
, which was introduced in #26476.

The bootstrap script is run during compilation but the CLI option is passed at runtime.

@joyeecheung is this something we should even be doing? If so, how?

@devsnek
Copy link
Member

devsnek commented Feb 13, 2022

options are set at runtime. bootstrapping happens at build time. if something needs to check options it should not be part of bootstrapping.

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2022

I don't think this can land until we make --experimental-fetch a no op.

@RaisinTen
Copy link
Contributor Author

Ah, yes indeed. I'm closing this because re-exposing fetch when it becomes stable should be much easier to do than fixing git conflicts in this pr.

@RaisinTen RaisinTen closed this Feb 14, 2022
@RaisinTen RaisinTen deleted the add-fetch-to-bootstrap/browser.js branch February 14, 2022 16:10
@joyeecheung
Copy link
Member

A trick that you can do is to add it during bootstrap unconditionally and then removing it during pre-execution when the option is not on - but yeah it’s probably still too early to do this for fetch.

@RaisinTen
Copy link
Contributor Author

@joyeecheung yes, I did something similar in #41969.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetch should not be installed if no_browser_globals==true
5 participants