Replace computed MemberExpressions w/ string literal as key

[monsanto]

Cross-ref https://github.com/hughsk/envify/issues/20, ccing @yoshuawuyts

The first commit adds the requested feature. Unfortunately it causes a test to fail. The test however is broken: it is supposed to test whether envify ignores assignments to environment variables, but it only passed before because the test happened to use computed MemberExpressions. The second commit adds additional tests that show that envify's previous behavior was broken.

I don't have time at the moment to fix the assignment bug, perhaps someone else can pitch in.

[yoshuawuyts]

Guess most of the code looks ok. Haven't tested it. Only thing that needs to happen is make the tests pass (don't have time to do that either now.)

[zertosh]

@monsanto You're right about the assignment bug. All you have to do is check that if the parent node is an AssignmentExpression, that we are not on the left side of that. Adding this && !(path[0] && path[0].type === Syntax.AssignmentExpression && path[0].left === node) to the already gross visitor test fixes it. So:

    return (
      node.type === Syntax.MemberExpression
      && !(path[0] && path[0].type === Syntax.AssignmentExpression && path[0].left === node)
      && (!node.computed && node.property.type === Syntax.Identifier
          || node.computed && node.property.type === Syntax.Literal
                           && typeof node.property.value === 'string')
      && node.object.type === Syntax.MemberExpression
      && node.object.object.type === Syntax.Identifier
      && node.object.object.name === 'process'
      && node.object.property.type === Syntax.Identifier
      && node.object.property.name === 'env'
    )

Side note: The visitor test doesn't have to be that complicated. In fact, we could probably speed up the traversal a bit by only checking for node.type === Syntax.MemberExpression, then performing the rest of the checks in visitProcessEnv. That way, we can return false; if the criteria doesn't match and avoid re-traversing that branch.

[zertosh]

@monsanto The fix for the assignment bug is in master. If you rebase your changes, I'd be more than happy to review and merge this :smile:

[monsanto]

@zertosh awesome. i rebased, all tests pass for me

[zertosh]

@monsanto Thanks for the fast turnaround. While I was testing your code, I noticed yet another bug https://github.com/hughsk/envify/pull/24. But here are some points on your PR:

• typeof node.property.value === 'string' isn't really necessary because when using bracket notation keys are cast to strings. So, even if value is boolean, or null, or whatever, that's still ok. process.env[false] is the same as process.env['false'].
• Maybe for legibility sake use a ternary to keep short?
  • && node.property.type === (node.computed ? Syntax.Literal : Syntax.Identifier)

[monsanto]

@zertosh OK, those are in.

[zertosh]

@monsanto Sorry for the delay :disappointed:

[zertosh]

Published as envify@3.4.0