Skip to content

(svelte2tsx)blank possibly operator or property access near end of mustache before svelte compile #786

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

Merged
merged 9 commits into from
Feb 2, 2021

Conversation

jasonlyu123
Copy link
Member

Run a simple scan before sending the code to the svelte compiler. Because it's mostly a syntax error to have an operator right before the end of the mustache tag. This would allow the errored syntax to be emitted untouched. Allowing better dx. But this workaround should only apply to a syntax error. So we have to check if any of it is a valid syntax.
for example:

{a?.}
{a. }
{a ? }
{a &&}
{a+}

#538

@dummdidumm
Copy link
Member

This looks very nice. I'm trying to think of situations where this would break otherwise correct output.
If we want to be even more safe, we could try to do two passes: One without that scan, if that works -> go on; if it doesn't -> try again with the scan. What do you think?

@jasonlyu123
Copy link
Member Author

Kind of wanted to just go for it XD.

@dummdidumm
Copy link
Member

I think we need this to be configurable. svelte-check uses svelte2tsx, too, and in this case we want these errors to appear.

@jasonlyu123
Copy link
Member Author

I agree. What about emitOnTemplateError in the option parameter of svelte2tsx

@dummdidumm
Copy link
Member

Sounds good! It should be false by default. We also somehow need to enhance the tests so that you can invoke svelte2tsx with arbitrary options and test whether it fails or not. Maybe check if there's test.js in the test case folder and if yes invoke that method within instead.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! One more thing, then I we are good to merge I think: You put the test.js in the same folder as the tests for a conversion from svelte to tsx/jsx. The test utils are written such that if there's a test.js, the other tests will not be done anymore. So I think you need to split out the the test.js tests into separate folders. This would also make it more clear that these are two different tests.

@jasonlyu123
Copy link
Member Author

Do you mean a folder in the test folder dedicated to the custom tests?

@dummdidumm
Copy link
Member

I meant instead of

html2jsx/edit-moustache
  | test.js
  | input.svelte
  | expected.jsx

Two sparate test cases

html2jsx/edit-moustache
  | input.svelte
  | expected.jsx
html2jsx/moustache-error
  | test.js

(same for the svelte2tsx test)

@jasonlyu123
Copy link
Member Author

jasonlyu123 commented Feb 2, 2021

This means we'll make the default test use the new option. And the custom one uses the default option to test it throw error?

@dummdidumm
Copy link
Member

Oh I noticed just now that test.js actually contains two test cases, I thought it was only one and that the svelte->jsx/tsx test would be swallowed by that. Nevermind then 😄 I'll merge this then if you don't have any objections.

@jasonlyu123
Copy link
Member Author

Ok! no other to adjust.

@dummdidumm dummdidumm merged commit d993b25 into sveltejs:master Feb 2, 2021
dummdidumm pushed a commit that referenced this pull request Feb 16, 2022
Related to #538, #786 and #1302

This PR added a utility to check if there's trailing property access behind an expression. The Reason we needed this is that we blank the operators before parsing. So the operator won't exist in the AST. We need to check it so that the expression transformation also include it.

This should only be called with an expression that is a direct child of a template node. And since we only go backwards from the end of the moustache (}), it only makes sense to call this with the expression that might end directly before the end of the moustache.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants