Transforms JS SDK: plumb environ down into process.env
#20313
Merged
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
oleiman
commented
Jun 27, 2024
oleiman
commented
Jun 27, 2024
oleiman
force-pushed
the
xform/sdk/core-2770/js-process-env
branch
3 times, most recently
from
f1d5afb
to
dc25279
Compare
oleiman
force-pushed
the
xform/sdk/core-2770/js-process-env
branch
2 times, most recently
from
28052c1
to
76993e6
Compare
oleiman
commented
Jul 15, 2024
oleiman
force-pushed
the
xform/sdk/core-2770/js-process-env
branch
2 times, most recently
from
81ce854
to
9eee29f
Compare
rockwotj
reviewed
Jul 15, 2024
src/transform-sdk/js/js_vm.cc
Outdated
| return result; | ||
| } | ||
|
|
||
| auto process_builder = class_builder<process>(_ctx.get(), "Process"); |
There was a problem hiding this comment.
Can we not create a class but just make a plain object with a single property of env? I don't see the value of a class here.
There was a problem hiding this comment.
- just realized dot notation can be used interchangeably with brackets for arbitrary objects 🤦
- I was thinking that people might come to expect some of the other junk offered by Process, but it won't be hard to add the class later if needed, esp. given (1).
There was a problem hiding this comment.
- It took me time to realize this too. Magic of JS!
- YAGNI, but yes it's not hard to add if so
src/transform-sdk/js/js_vm.cc
Outdated
| @@ -394,6 +395,50 @@ class console { | |||
| FILE* _info_stream; | |||
| FILE* _err_stream; | |||
| }; | |||
|
|
|||
| class process { | |||
| using map_t = std::unordered_map<std::string, std::string>; | |||
There was a problem hiding this comment.
Can we add NODE_ENV=production if there is no user environment variable set for the NODE_ENV key?
oleiman
force-pushed
the
xform/sdk/core-2770/js-process-env
branch
from
9eee29f
to
784418f
Compare
|
force push CR |
Reflects contents of `environ` at transform init Signed-off-by: Oren Leiman <[email protected]>
oleiman
force-pushed
the
xform/sdk/core-2770/js-process-env
branch
from
784418f
to
11e1624
Compare
|
force push pesky |
rockwotj
approved these changes
Jul 15, 2024
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.
This PR adds a
Processclass and injects an instance into the global namespace asglobalThis.process. Currently we implement a top-levelenvobject that reflects the contents ofenvironat init time, but in principle this could take on other process stuff.Backports Required
Release Notes