Simplifiy tracking of valid JSX positions #13891
Conversation
JLHwung
added
PR: Internal 🏠
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5267395:
|
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49601/ |
JLHwung
force-pushed
the
parser-state-cleanup
branch
from
50f3425
to
5267395
Compare
| this.state.type = type; | ||
| // the prevType of updateContext is required | ||
| // only when the new type is tt.slash/tt.jsxTagEnd | ||
| // $FlowIgnore |
There was a problem hiding this comment.
We might consider marking it as optional?
There was a problem hiding this comment.
I am working on completely removing updateContext() in base parser and jsx parser, at least currently we don't need updateContext in the base parser so hopefully replaceToken can be moved to the jsx plugin.
nicolo-ribaudo
dismissed
their stale review
I'm dropping my approval not because I have some questions
| if (liberal) { | ||
| // If the current token is not used as a keyword, set its type to "tt.name". | ||
| // This will prevent this.next() from throwing about unexpected escapes. | ||
| this.state.type = tt.name; | ||
| if (tokenIsKeyword) { | ||
| this.replaceToken(tt.name); |
There was a problem hiding this comment.
Is updateContext needed when replacing a keyword with tt.name? (I don't think it is).
Because we could just to this.state.type = tt.name here (and maybe this.state.exprAllowed = false, even if I don't see why it would be needed) and keep the prevType handling all in the JSX plugin (to avoid needing the type error suppression comment).
There was a problem hiding this comment.
Uh, or maybe it would break this:
foo.else
<X>2There was a problem hiding this comment.
The updateContext is currently required by the jsx plugin, to disable JSX element forming in the following situations, where a keyword token was re-interpreted as an identifier token and thus it can not start a JSX element:
x.case <jsx ...
x?.case <jsx ...
class C { case <jsx ...
There was a problem hiding this comment.
Approving since updateContext will be removed anyway - #13891 (comment)
|
The CI failure is fixed in #13923 |
nicolo-ribaudo
changed the title
state.inPropertyName and simplifies state.canStartJSXElement tracking
|
I rewrote the commit title to fit the max length; the original title is in the commit description. |
github-actions
bot
added
the
outdated
This PR can be reviewed by commits.
The first commit removes
state.inPropertyName. The intend ofinPropertyNameis to disallow JSX tag forming after a property name, which would have been a method with type parameters. We already havestate.exprAllowedfor this purpose so we can setstate.exprAllowedafter the property name is consumed.The second commit renames
exprAllowedtocanStartJSXElementfor clarity.The third commit handles the
canStartJSXElementinparseIdentifierName: when we see a liberal keyword, we mark the token type astt.namebut did not update thecanStartJSXElement.In the fourth commit we add a new
replaceTokenmethod which only updates current token and token context, so we don't have to carecanStartJSXElementin the base parser, they will be handled by the JSX pluginBecause
parseIdentifierNameis a popular path, the last commit ensures that we callreplaceTokenonly when we have to do so.