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

Update acorn-jsx #345

Closed
kaicataldo opened this issue Jun 22, 2017 · 10 comments
Closed

Update acorn-jsx #345

kaicataldo opened this issue Jun 22, 2017 · 10 comments
Labels

Comments

@kaicataldo
Copy link
Member

kaicataldo commented Jun 22, 2017

As described here, we should upgrade acorn-jsx to v4.0.1. Making an issue in case someone wants to take this on :)

@soda0289
Copy link
Member

This would be a breaking change since it now uses JsxText node type for stings inside elements. We could make an option to revert back to the old Literal node type, similar to what was done with the typescript-eslint-parser.

I can work on this next week sometime.

See here eslint/eslint#8591

@kaicataldo
Copy link
Member Author

Ah, good to know. Yes, having an option to use the old type (and have that be the default so it's not a breaking change) definitely makes sense!

@kaicataldo
Copy link
Member Author

Or, actually, do we care if it's a breaking change? As long as we have the option to turn it off, I suppose it doesn't matter, doesn't it?

I'd rather support the correct thing by default and then turn it off in ESLint until v5.

@soda0289
Copy link
Member

As long as we have an option to use Literal node types instead of JsxText it would not be a breaking change for eslint.

We may want to allow for the eslint parserOptions to override the option in case some plugin developers would like to ensure their rules work with the new AST node type. This would make writing tests for both cases a lot easier.

@platinumazure
Copy link
Member

What about plugin rule developers? I think we need to avoid a breaking change here so that plugin authors can decide when to switch to the new node type and release that behind a major release (and espree should also switch the default node type used in a major release).

@soda0289
Copy link
Member

soda0289 commented Jun 22, 2017

By allowing for an option to configure which node type is used then an eslint plugin developer can ensure their rule will work with both node types. I think most jsx plugins will want to be compatible with eslint <=v4 and v5 since the change is fairly trivial.

It might confuse users having an option, like useJSXText, so I can understand not wanting to add an option. But I think it will make development and testing of rules easier when rule authors want to support both node types.

@platinumazure
Copy link
Member

I'm in favor of having an option (and supporting this in ESLint as well would not be a bad thing). I just don't think the default emitted node type should change without a major release.

@kaicataldo
Copy link
Member Author

Just to be clear, I'm advocating for a major release if we make a breaking change.

@soda0289
Copy link
Member

I think we should change the AST of espree to match the JSX spec, which would be a breaking change. Release a new major version of espree and update eslint to use the new version but with an option to use the old Literal node type. That way eslint does not break any existing rules and espree will produce an AST matching the JSX spec

@not-an-aardvark
Copy link
Member

Since we're probably going to do a major release of ESLint sometime in the next few months, I think the best solution might be to do a major release of espree at the same time, with this change included. That way we won't need to implement an option for this. (We could also start drop support for Node 4 in espree.)

aladdin-add added a commit to aladdin-add/espree that referenced this issue Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants