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

Duplicate keys in object literals transpile to ES5-incompatible JS #2462

Closed
jussi-kalliokoski opened this issue Sep 30, 2015 · 7 comments
Closed
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Milestone

Comments

@jussi-kalliokoski
Copy link

The following ES6 code:

var x = {x: 1, x: 2};

generates the following code:

"use strict";

var x = { x: 1, x: 2 };

which will result in an error for ES5-compliant engines. I'm not sure what should be done here instead as duplicate keys are back to being valid in strict mode in ES6, but maybe something like this:

"use strict";

var x = { x: 1 };
x.x = 2;

This bug also occurs with JSX:

var x = <x x={1} x={2} />;

becomes

"use strict";

var x = React.createElement("x", { x: 1, x: 2 });
@sebmck sebmck added this to the 6.0.0 milestone Sep 30, 2015
@sebmck
Copy link
Contributor

sebmck commented Sep 30, 2015

This will have to be done with another transform. Seems easy enough.

@jussi-kalliokoski
Copy link
Author

Yeah, something like this seems to produce working code (horribly inefficiently though) if placed after the JSX transforms and before other ObjectExpression related transforms:

export default function ({Plugin, types: t}) {
  function getName (key) {
    if ( t.isIdentifier(key) ) { return key.name; }
    return key.value.toString();
  }

  return new Plugin('duplicate-property-keys-to-es5', {
    visitor: {
      ObjectExpression (node) {
        const plainProps = node
          .properties
          .filter(prop => !t.isSpreadProperty(prop))
          .filter(prop => !prop.computed);

        plainProps
          .filter((prop, index) =>
            plainProps
              .slice(0, index)
              .some(previousProp =>
                getName(prop.key) === getName(previousProp.key)
              )
          )
          .forEach(prop => {
            prop.computed = true;
            prop.key = t.literal(getName(prop.key));
          });
      }
    }
  });
}

@petetnt
Copy link

petetnt commented Oct 1, 2015

Maybe this should be an option with a sensible default (IMHO default being error, because one could argue that 99% of the times duplicate key is an (user) error)? Others being something like warn (current behavior + warning) and overwrite (which is non-strict mode ES5 default)? Duplicated property values are allowed in ES6, though I think the restriction was removed because of duplication being possible due to computed property values on runtime being possible, right? MDN says that:

In ECMAScript 5 strict mode code, duplicate property names were considered a SyntaxError. With the introduction of computed property names making duplication possible at runtime, ECMAScript 6 has removed this restriction.

@jussi-kalliokoski
Copy link
Author

I think that sounds more like the job of a linter - there's plenty of other silly things you can do with the language that babel doesn't warn nor throw for. IMO what babel should do here is just produce valid JS.

@petetnt
Copy link

petetnt commented Oct 4, 2015

Yeah, I was thinking about that. I have always set my linters to throw on duplicated keys (and other duped ones like variable names) and I do agree that babel should produce is valid JS and that there are tons of silly things that are better left for the linter or coder to handle.

But then again the expected outcome is hard to say in a transpiling cases like with babel: duplicate keys is most likely something that is not expected (especially with non-runtime, precomputed values). But you are right, it should be left for linters to handle.

@Macil
Copy link
Contributor

Macil commented Oct 29, 2015

Perhaps this could be simple to implement if duplicate keys were treated the same way as computed keys. var x = {o: 5, o: 6}; could be treated like var x = {o: 5, ["o"]: 6}; and transpiled to the same thing using the same logic.

@babel-bot
Copy link
Collaborator

Comment originally made by @agentme on 2016-01-15T22:39:26.000Z

I see now that jussi-kalliokoski had already posted code to do my idea. [[ https://github.com/AgentME/babel-plugin-transform-es2015-duplicate-key-fix | I've just tweaked the code a little ]] and published it on NPM as "babel-plugin-transform-es2015-duplicate-key-fix", and then I published a fork of the es2015 preset that includes it as "babel-preset-es2015-dupkeyfix".


Comment originally made by @amasad on 2016-02-03T01:21:50.000Z

Just exclude the strict plugin which or use strict: false option if you want to use babel in non strict mode (which will allow duplicate keys)


Comment originally made by @hzoo on 2016-03-02T02:04:59.000Z

Merged #3280

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

No branches or pull requests

5 participants