acornjs / acorn-jsx

Alternative, faster React.js JSX parser
MIT License
646 stars 72 forks source link

`JSXSpreadAttribute` not supported when parsing with `ecmaVersion: 5` #40

Closed kaicataldo closed 8 years ago

kaicataldo commented 8 years ago

First, wanted to say thanks for working on this. I know the ESLint community really appreciates us supporting JSX out of the box!

We've run into the following issue on ESLint.

Running the following:

'use strict';

require('babel-register')({
  presets: [ 'es2015' ]
});

var acorn = require('acorn-jsx/inject')(require('acorn'));
var ast = acorn.parse(
  '<div {...this.props} />',
  {
    ecmaVersion: 5,
    plugins: { jsx: true }
  }
);

console.log(JSON.stringify(ast, null, 4));

yields the following:

/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn/dist/acorn.js:2223
    throw err
    ^

SyntaxError: Unexpected token (1:6)
    at Parser.pp$4.raise (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn/dist/acorn.js:2221:15)
    at Parser.pp.unexpected (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn/dist/acorn.js:603:10)
    at Parser.pp.expect (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn/dist/acorn.js:597:28)
    at Parser.pp.jsx_parseAttribute (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn-jsx/inject.js:272:12)
    at Parser.pp.jsx_parseOpeningElementAt (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn-jsx/inject.js:289:33)
    at Parser.pp.jsx_parseElementAt (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn-jsx/inject.js:310:31)
    at Parser.pp.jsx_parseElement (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn-jsx/inject.js:359:17)
    at Parser.parseExprAtom (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn-jsx/inject.js:381:23)
    at Parser.pp$3.parseExprSubscripts (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn/dist/acorn.js:1715:21)
    at Parser.pp$3.parseMaybeUnary (/Users/kaicataldo/Code/test/acorn-test/node_modules/acorn/dist/acorn.js:1692:19)

From what I've read it looks like JSX Spread Attributes should be allowed in ES5, since it's part of the JSX spec. Does that sound right to you? If so, any idea if the fix would be in this plugin or in Acorn itself?

RReverser commented 8 years ago

Thanks for the kind words!

since it's part of the JSX spec

On one hand, yes, on another JSX spec just doesn't have versioning that ECMAScript does, so ecmaVersion is used in both contexts. The thing is that ... operator is only allowed for ES6+ on tokenizer level since that's the version where it first appeared and was allowed.

In order to change this, it would indeed require various modifications of Acorn itself (to present new ecmaVersion checks in various places of parser instead of single check in tokenizer).

As for Babel approach mentioned in the original issue, it simply removed ecmaVersion option when forked Acorn to Babylon, and instead always parses the latest ECMAScript version, and thus works even with only React preset. This is exactly the same as setting ecmaVersion to 7 in Acorn.

Let me know if this makes sense to you.

kaicataldo commented 8 years ago

Yes, it does - appreciate the explanation! Thanks for your help.

JakeSidSmith commented 8 years ago

@RReverser I know it's a little over simplified, but could something like my test mentioned here: https://github.com/eslint/eslint/issues/6852#issuecomment-242461386 not work?

I know it'd need adjustments to acorn itself, but providing context for being inside JSX tags should allow thee use of the spread operator conditionally.

If there's more complexities, could you explain them?

RReverser commented 8 years ago

@JakeSidSmith Acorn should stay unopinionated about its plugins. Having something like typeof this.options.plugins.jsx !== 'undefined' && this.insideJSXTag is not acceptable in the core.

Given my explanation above, can you please tell why setting ecmaVersion to 6 doesn't work for you? (given that it's the same what Babel, which you mentioned in the original issue, does when parsing the code).

JakeSidSmith commented 8 years ago

@RReverser We've recently started using React on an older project, that's written in ES5. We don't intend to begin using ES6 any time soon.

If we must run eslint with ES6 then any bits of code that are considered okay in ES6 but not five, may slip under the radar, and are not compiled by babel (as we are only using the React preset). Babel just ignores these presumably, because it assumes you intended to use uncompiled ES6 code.

This has caused us to have bits of code get into master, that do not run in some of the browsers we support (as the ES6 spec is not completely implemented everywhere yet).