Skip to content
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

Keep comments in event handlers #106

Closed
wants to merge 2 commits into from

Conversation

trbrc
Copy link

@trbrc trbrc commented Jul 1, 2020

Fixes #96. Includes test that failed before, but is green now.

@trbrc
Copy link
Author

trbrc commented Jul 9, 2020

In case it helps the review of this PR, here’s a short summary of what the problem was and why this is the solution. Apparently Prettier handles comments in a special way. As far as I can understand, since the formatting will change line wraps, it can affect the legal placement of end-of-line comments in significant ways. So it appears that the comments are collected separately from the main AST and put back in valid locations in a later phase.

To handle expressions in template markup, prettier-plugin-svelte wraps the expression in parentheses. (I guess this is so the parser doesn’t interpret a function expression as a function statement?). When removing these parens again it only returns the expression AST without the comments, which must be an oversight. The result is that the comments are lost completely in the output.

The fix simply picks out the comments from the AST and includes them in the returned object.

Let me know if there’s anything else I can do to push this PR along.

@dummdidumm
Copy link
Member

This keeps the comments, but /* ... */ somehow get prepended to the next code line:

<button
    on:click={() => {
        /* Event handler with comment */
        const a = 1;
    }}>
    toggle
</button>

gets

<button
    on:click={() => {
        /* Event handler with comment */ const a = 1;
    }}>
    toggle
</button>

Could you add a test for this and check why this happens? If you don't find out why I'm inclined to still merge because a comment prepended is still better than no comment.
Could you also add a test for // ... comments?

@ehrencrona
Copy link
Contributor

I just tested this; the // unfortunately doesn't work and the reason is the same as that newline disappearing: the expressionParser adds parenthesis around the expression, but that makes all the offsets in the parser wrong, so when it tries to retrieve the commenting style ( */) it actually retrieves (*/\n). This is even worse with // which it would turn into /. That was quite messy to debug.

I created #133 that fixes the expression parser too. or you can add those fixes to this PR if you prefer.

@dummdidumm dummdidumm closed this in 4562757 Sep 3, 2020
@dummdidumm
Copy link
Member

Thank you for your effort @trbrc ! I decided to go with @ehrencrona's PR.

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 are removed from inline event listeners
3 participants