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

Don't ignore whitespace in expressions #930

Merged
merged 8 commits into from
Jan 2, 2024
Merged

Conversation

MoustaphaDev
Copy link
Member

@MoustaphaDev MoustaphaDev commented Dec 28, 2023

Changes

Testing

  • Adjusted whitespace in a few tests
  • Added a printer test to make sure empty expressions that contain whitespace are handled like before.
  • Added a TSX output test to ensure the whitespace in expressions is respected

Docs

N/A bug fix

Copy link

changeset-bot bot commented Dec 28, 2023

🦋 Changeset detected

Latest commit: 5f41338

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

This PR includes changesets to release 1 package
Name Type
@astrojs/compiler 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

@MoustaphaDev

This comment was marked as outdated.

This comment was marked as outdated.

Comment on lines +319 to +323
clean := ""
if n.FirstChild != nil {
clean = strings.TrimSpace(n.FirstChild.Data)
}
if n.FirstChild == nil || clean == "" {
Copy link
Member Author

Choose a reason for hiding this comment

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

We now need to check for whitespace too as the parser doesn't remove them in expressions anymore.

@MoustaphaDev MoustaphaDev marked this pull request as ready for review December 28, 2023 22:59
Comment on lines -233 to -235
} else if expressionOnlyHasComment(n) {
// we do not print expressions that only contain comment blocks
return
Copy link
Member Author

@MoustaphaDev MoustaphaDev Dec 28, 2023

Choose a reason for hiding this comment

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

Don't ignore expressions with only comments in the TSX output

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I wonder why Nate added that check initially?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, I was wondering the same as that would unexpectedly remove TS directives like @ts-ignore in this case.
Pinging @natemoo-re, maybe he could provide some background

Copy link
Member

Choose a reason for hiding this comment

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

I honestly don't remember the context! Guessing it was just a copy + paste from the JS output which has special handling for this case.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Admittedly not super familiar with the logic around these parts, but this looks good to me!

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.

🐛 BUG: [TSX] Multiline comments are handled weirdly
3 participants