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

properties created using Symbols in process.env are not intercepted #9429

Closed
AnnaMag opened this issue Nov 2, 2016 · 7 comments
Closed

properties created using Symbols in process.env are not intercepted #9429

AnnaMag opened this issue Nov 2, 2016 · 7 comments
Labels
process Issues and PRs related to the process subsystem.

Comments

@AnnaMag
Copy link
Member

AnnaMag commented Nov 2, 2016

  • Version: v6.2.0
  • Platform: x86_64
  • Subsystem:

switching off the flag PropertyHandlerFlags::kOnlyInterceptStrings in node.cc
causes Type Error when intercepting via Symbols on process.env collection,
which is strongly typed for Strings (error in EnvSetter in node.cc).
to reproduce: switch off the flag and run /test/parralel/test-v8-interceptStrings-not-Symbols.js

@addaleax addaleax added the process Issues and PRs related to the process subsystem. label Nov 2, 2016
@addaleax
Copy link
Member

addaleax commented Nov 2, 2016

I am not sure… what behaviour would you expect?

@AnnaMag
Copy link
Member Author

AnnaMag commented Nov 2, 2016

The vm module intercepts both Strings and Symbols, while the current version of the process.env supports only Strings. This is to ensure that the properties are treated in the same way, no matter how they were created.
The test I referred to throws:
TypeError: Cannot convert a Symbol value to a string
with the flag off.

@addaleax
Copy link
Member

addaleax commented Nov 2, 2016

You’re right. But what do you think should happen when somebody does process.env[someSymbol] = 42? Do you think the property should get set to '42', or do you think it should throw, or do you think the current behaviour is fine but should be documented? I can see arguments for all of these perspectives. 😄

@cjihrig
Copy link
Contributor

cjihrig commented Nov 2, 2016

Also keep in mind that process.env is already a bit of a special snowflake, so it doesn't need to behave like a typical object.

@AnnaMag
Copy link
Member Author

AnnaMag commented Nov 3, 2016

Having a flag makes strings optional + Symbols are commonly used, so my thinking is that it 'should' read and set values. That said, my understanding of the design choices here is limited and curious to hear all the perspectives.

@bnoordhuis
Copy link
Member

process.env maps directly to the C environment and that only supports strings. If symbols are to be supported, they have to be turned into strings, there is no way around that.

The default stringification for symbols isn't very useful (Symbol().toString() == 'Symbol()'). Perhaps throwing an exception is the best course of action. It protects users against passing a symbol as the key by accident.

Setting a symbol as the value already throws an exception (the 'Cannot convert a Symbol value to a string' error mentioned above.) Throwing on symbol keys would make it symmetrical.

@jasnell
Copy link
Member

jasnell commented Nov 3, 2016

I'd definitely be in favor of having a more specific TypeError message when using a Symbol as the key or value in process.env.

cjihrig added a commit to cjihrig/node that referenced this issue Nov 7, 2016
This commit causes process.env to throw when a symbol is used as
either a key or a value.

Fixes: nodejs#9429
PR-URL: nodejs#9446
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
This commit causes process.env to throw when a symbol is used as
either a key or a value.

Fixes: nodejs#9429
PR-URL: nodejs#9446
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants