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 slicing for whitespace-stripping comments #1717

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chancancode
Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Feb 21, 2025

This PRmain
Dev
582K └─┬ .
169K   ├── runtime
159K   ├── syntax
100K   ├── compiler
 58K   ├── opcode-compiler
 27K   ├── manager
 19K   ├── validator
 11K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.5K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
582K └─┬ .
169K   ├── runtime
159K   ├── syntax
100K   ├── compiler
 58K   ├── opcode-compiler
 27K   ├── manager
 19K   ├── validator
 11K   ├── program
8.9K   ├── reference
7.2K   ├── destroyable
6.3K   ├── util
4.3K   ├── node
3.4K   ├── global-context
2.5K   ├── wire-format
1.0K   ├── vm
969B   ├── encoder
844B   ├── vm-babel-plugins
606B   └── owner
Prod
230K └─┬ .
 69K   ├── syntax
 63K   ├── runtime
 48K   ├── compiler
 18K   ├── opcode-compiler
7.9K   ├── manager
4.8K   ├── program
4.0K   ├── validator
3.6K   ├── reference
2.4K   ├── util
2.1K   ├── node
1.6K   ├── wire-format
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner
230K └─┬ .
 69K   ├── syntax
 63K   ├── runtime
 48K   ├── compiler
 18K   ├── opcode-compiler
7.9K   ├── manager
4.8K   ├── program
4.0K   ├── validator
3.6K   ├── reference
2.4K   ├── util
2.1K   ├── node
1.6K   ├── wire-format
1.5K   ├── destroyable
737B   ├── vm
594B   ├── global-context
516B   ├── encoder
469B   ├── vm-babel-plugins
155B   └── owner

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 <[email protected]>
@chancancode chancancode force-pushed the check-for-tilde-on-comment-2 branch from 1283e03 to 5256314 Compare February 21, 2025 22:38
@NullVoxPopuli
Copy link
Contributor

https://github.com/glimmerjs/glimmer-vm/actions/runs/13466445316/job/37633202706?pr=1717#step:4:5753

image

Same as on main, so we're good here 🎉

thanks for fixing this!

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] Ember's template compiler on 3.25 complains about ~ in a comment
2 participants