Latest version of parser #917
Conversation
| posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->32:4] | ||
| posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->30:12] | ||
| JSX <di:[30:10->30:12] > _children:None | ||
| posCursor:[30:12] posNoWhite:[30:11] Found expr:[15:8->33:2] |
There was a problem hiding this comment.
which location is correct?
There was a problem hiding this comment.
Not sure actually. Think it's a problem?
There was a problem hiding this comment.
Depends on whether they're wrong or fixed now.
There was a problem hiding this comment.
Not sure why the output is different, but it's this test:
<div>
{React.string(someProp)}
<div> {React.null} </div>
// {someString->st}
// ^com
// {"Some string"->st}
// ^com
// {"Some string"->Js.String2.trim->st}
// ^com
// {someInt->}
// ^com
// {12->}
// ^com
// {someArr->a}
// ^com
// <di
// ^com
</div>I think when completing di, it finds the following /div and now considers that "div" as a prop of di, while before it did not.
The one thing that comes to mind is that the treatment of nested jsx could have changed with the jsx mode pops etc in this:
rescript-lang/rescript#6609
@tsnobip any thoughts?
Not sure there's anything to do, just double checking if anything comes to mind. Otherwise we can just go ahead.
There was a problem hiding this comment.
it is totally possible that my PR accidentally created some regressions, do you have an example in mind of syntax that should not be recognized but is now? I could add tests. I'm not really happy about working with the syntax parser right now, especially around mode, I think it can introduce bugs that are hard to spot.
There was a problem hiding this comment.
Hmm, we should likely add tests around this. I'm not sure what's the best way to do that.
Basically, before, jsx mode tended to "leak" outside of jsx, which created unwanted behavior like accepting hyphens in identifiers outside of jsx. I tightened the jsx mode to avoid that, but I might have restricted it too much. I might have popped jsx mode one time too many.
There was a problem hiding this comment.
It could be that the difference here is because jsx mode is now correctly restored while before it was not.
In which case that's great.
There was a problem hiding this comment.
I've tested this for a bit and I can't find any obvious issues with it. Not tests fail either from what I see. I'd like to merge this, because 2 other PRs depend on this, but I'd also not want to drop the ball here on if this change actually is problematic. How can we investigate it further? Add tests in the syntax for nested JSX mode?
There was a problem hiding this comment.
This can be merged.
One can add a couple of tests of nested jsx separately in the compiler.
There was a problem hiding this comment.
@tsnobip would you be up for adding tests for various forms of nested JSX in the compiler repo?
* latest version of parser * changelog
Closes #915