bootstrap: consolidate global properties definition #43357
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
c++
legendecas
force-pushed
the
global-process
branch
from
09634a7
to
dd593ec
Compare
legendecas
force-pushed
the
global-process
branch
from
dd593ec
to
ba792a8
Compare
aduh95
approved these changes
Jun 9, 2022
aduh95
added
author ready
aduh95
reviewed
Jun 9, 2022
github-actions
bot
removed
the
request-ci
aduh95
reviewed
Jun 9, 2022
legendecas
force-pushed
the
global-process
branch
from
ba792a8
to
89b291b
Compare
aduh95
reviewed
Jun 9, 2022
legendecas
force-pushed
the
global-process
branch
from
89b291b
to
39fbce4
Compare
github-actions
bot
removed
the
request-ci
joyeecheung
reviewed
Jun 10, 2022
lib/internal/bootstrap/node.js
Outdated
| configurable: true | ||
| }); | ||
|
|
||
| // globalThis.process is been installed at `internal/bootstrap/pre_execution.js`. |
There was a problem hiding this comment.
Suggested change
| // globalThis.process is been installed at `internal/bootstrap/pre_execution.js`. | |
| // globalThis.process will be installed at `internal/bootstrap/pre_execution.js`. |
There was a problem hiding this comment.
But why are we removing these anyway? Removing these results in a breaking change for the embedders, which is why the tests have to be updated.
There was a problem hiding this comment.
I was reviewing the installation point of globalThis.process and globalThis.Buffer. It can be more intuitive to install them once rather than duplicate the installation.
It is true that removing can cause breaking changes on embedders with custom entry points. What do you think about moving the actual installation here?
There was a problem hiding this comment.
I think the reason that they are overriden in pre_execution is that we thought we were going to make this optional for ESM (probably behind flags), which didn't seem to happen...
There was a problem hiding this comment.
Yeah, that's true. From what I could tell, the current shape of the property descriptor is not going to be changed in anytime soon.
legendecas
removed
the
author ready
legendecas
force-pushed
the
global-process
branch
2 times, most recently
from
c64d580
to
39394c2
Compare
lpinca
approved these changes
Jun 10, 2022
aduh95
reviewed
Jun 10, 2022
`globalThis.process` and `globalThis.Buffer` has been re-defined with a getter/setter pair. `atob` and `bota` are defined as enumerable properties according to WebIDL definition.
legendecas
force-pushed
the
global-process
branch
from
39394c2
to
9c4ae03
Compare
15 tasks
joyeecheung
approved these changes
Jun 13, 2022
github-actions
bot
removed
the
request-ci
|
Landed in 511e289 |
legendecas
added a commit
that referenced
this pull request
Jun 13, 2022
`globalThis.process` and `globalThis.Buffer` has been re-defined with a getter/setter pair. `atob` and `bota` are defined as enumerable properties according to WebIDL definition. PR-URL: #43357 Refs: #26882 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
16 tasks
danielleadams
pushed a commit
that referenced
this pull request
Jun 14, 2022
`globalThis.process` and `globalThis.Buffer` has been re-defined with a getter/setter pair. `atob` and `bota` are defined as enumerable properties according to WebIDL definition. PR-URL: #43357 Refs: #26882 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
globalThis.processandglobalThis.Bufferhas been re-defined witha getter/setter pair.
atobandbotaare defined as enumerable properties according toWebIDL definition.
Refs: #26882
Refs: #37786 (comment)