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

Fix source checking when parsing {{~!~}} and the like. #1430

Closed
wants to merge 1 commit into from

Conversation

navels
Copy link
Contributor

@navels navels commented May 26, 2023

This fixes a warning that derives from several of these statements in ember-prism, which are intended to strip whitespace without otherwise doing anything:

{{~! ~}}

Presently these result in a compiler warning when building ember-prism or any addon which consumes it:

unexpectedly found "! ~" when slicing source, but expected " "
unexpectedly found "! ~" when slicing source, but expected " "
unexpectedly found "! ~" when slicing source, but expected " "
unexpectedly found "! ~" when slicing source, but expected " "

Fixes one cause of emberjs/ember.js#19392

When fixing this I decided some tests would be nice to add even though they don't cover this fix. Not really thinking it is worth a test specifically targeting this case, though.

@acorncom
Copy link
Member

I've been helping a client with some upgrades recently and found this message went away once the app was upgraded to 5.12. Probably can close now?

@boris-petrov
Copy link
Contributor

I am on 5.12 and I still very much have these messages.

@boris-petrov
Copy link
Contributor

@NullVoxPopuli, @wycats - could you please take a look at this PR if it really fixes the issue at hand? And also the other one? This has been an issue for quite a while now...

chancancode added a commit that referenced this pull request Feb 21, 2025
Re-apply #1430 with a more robust implementation and tests

Also turns the warning into a `localAssert()` so it gets stripped
before reaching downstream consumers.

This relates to #1431 in that this removes the warning, so to the
extent that #1431 was motivated by sliencing the warning in Ember
builds, it avoids the issue. But I haven't investigated the root
cause of that issue and whether it is indicative of an actual bug
somewhere deeper.

Fixes emberjs/ember.js#19392

Co-authored-by: Lee Nave <lee.nave@ovation.io>
chancancode added a commit that referenced this pull request Feb 21, 2025
Re-apply #1430 with a more robust implementation and tests

Also turns the warning into a `localAssert()` so it gets stripped
before reaching downstream consumers.

This relates to #1431 in that this removes the warning, so to the
extent that #1431 was motivated by sliencing the warning in Ember
builds, it avoids the issue. But I haven't investigated the root
cause of that issue and whether it is indicative of an actual bug
somewhere deeper.

Fixes emberjs/ember.js#19392

Co-authored-by: Lee Nave <lee.nave@ovation.io>
@navels
Copy link
Contributor Author

navels commented Feb 24, 2025

Covered by #1717

@navels navels closed this Feb 24, 2025
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.

3 participants