-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
Comments
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 |
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! |
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. |
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. |
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). |
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. |
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. |
Just to be clear, I'm advocating for a major release if we make a breaking change. |
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 |
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 |
As described here, we should upgrade acorn-jsx to v4.0.1. Making an issue in case someone wants to take this on :)
The text was updated successfully, but these errors were encountered: