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 kNoBrowserGlobals flag for Environment #40356

Closed
wants to merge 1 commit into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Oct 7, 2021

This is the runtime equivalent of the configure --no-browser-globals build flag.

A runtime flag is needed because embedders may support multiple modes of Node.js that, Node.js may both run in a browser environment, and in an independent environment that has no browser globals. For example, Node.js script running in web page spawning a script with child_process.fork.

/cc @nodejs/embedders

@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 Oct 7, 2021
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done via the internal options infra so that it doesn't bypass the snapshot building checks - instead of skipping the installation of the browser globals during bootstrapping (which is meant to be stateless), we should remove them in pre-execution.

@joyeecheung
Copy link
Member

I created #40357 to move the embedder options to the runtime part of the bootstrap

@zcbenz
Copy link
Contributor Author

zcbenz commented Oct 7, 2021

Thanks for looking into this, I'll create a new PR after #40357 gets merged.

@zcbenz zcbenz closed this Oct 7, 2021
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.

None yet

3 participants