Skip to content

fix: allow leading and trailing comments in mustache tag #11866

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 7 commits into from
Jul 15, 2024

Conversation

BlueGreenMagick
Copy link
Contributor

Fixes #7456

This PR allows having leading and trailing comments in mustache expressions. This can be useful when disabling eslint for expression in expression, like following

<script lang="ts">
  export let num: number;
</script>

{num ?? num /* eslint-disable-line */ }

This PR does not change that having only comments in mustache does not work, such as {/* comment */} as often used in React.

trailingComments are attached to parent node instead when parent.end === node.end, so that checking for end position of 'js expression + comment' can be done by checking for end position of the root node's last trailing comment.

Further

If this PR is accepted, the print(ast) function of esrap (and perhaps code-red as well) needs to be changed. An assumption is made that there can only be at most 1 trailingComments, but this PR changes it to potentially have multiple comments. So it causes a minor bug where the 2nd+ trailing comments are not printed when printing js from ast.

https://github.com/Rich-Harris/esrap/blob/2b9561c679ed38cc216e7d481a323bfef387a022/src/handlers.js#L59-L61

If needed, I can create a PR for that as well.

this is required to correctly calculate expression's ending index correctly
with expr.trailingComments.at(-1).end
in `compiler/phases/1-parse/read/expression.js`
Copy link

changeset-bot bot commented Jun 1, 2024

🦋 Changeset detected

Latest commit: ab831f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

Thank you! Apologies for taking a while to get to this PR — it looks great and ready to go from my perspective, but I just want to get a +1 from @dummdidumm first in case there are any language-tools implications — hopefully this will self-fix once the updated version is released, but Prettier currently balks at this and the syntax highlighting is off:

image

@dummdidumm
Copy link
Member

Everything works - one has to run pnpm build to get the CJS version of the compiler (which language tools use) and then restart the Svelte extension to see such changes in effect.

@dummdidumm dummdidumm merged commit 81ed425 into sveltejs:main Jul 15, 2024
9 checks passed
trueadm pushed a commit that referenced this pull request Jul 16, 2024
Fixes #7456

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
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.

/* */ comments don't work in inline functions
3 participants