emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.44k stars 4.21k forks source link

[Bug] Ember's template compiler on 3.25 complains about ~ in a comment #19392

Open boris-petrov opened 3 years ago

boris-petrov commented 3 years ago

Running ember serve with ember-source 3.25 leads to:

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

These correspond to these two lines of HBS (they are in different files, I've just put them here for conciseness):

{{~!~}}
{{! comment ~}}

This didn't happen before. Not sure if this is a bug in the template compiler or what I did before was not supposed to work.

Funny thing is that these "errors" appear only sometimes. I can't seem to understand when they appear and when they don't.

urbany commented 3 years ago

I was also seeing these errors after upgrading to 3.25.1 but it stoppped after i deleted node_modules and installed it again

rwjblue commented 3 years ago

Hmm, this is odd indeed. It definitely sounds like a bug, but I'm not sure exactly what is going on.

rwjblue commented 3 years ago

I could definitely use a reliable reproduction if someone has the time to try and isolate one...

boris-petrov commented 3 years ago

@rwjblue - here. Clone, npm install, ember serve. You'll see the warnings in the console. I'm running Node v15.7.0.

P.S. There are obviously some caches at play because the second time I run ember serve the errors are gone. Even if I remove the dist directory. Perhaps something is left in /tmp, not sure. You can probably tell.

This is the HBS that used to cause the warnings:

{{! comment ~}}
<div></div>

<div>
  {{~!~}}
  <span></span>
</div>
rwjblue commented 3 years ago

Ya, we cache the compilation result into system temp. You can disable that behavior (likely causing an error on every rebuild) with CI=true environment variable.

boris-petrov commented 3 years ago

Well, in any case, that's concerning you mostly because you're fixing it. :smiley: You managed to reproduce it, right?

Turbo87 commented 3 years ago

FWIW we're seeing similar issues:

unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<form" when slicing source, but expected "class"
unexpectedly found "<form" when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<asid" when slicing source, but expected "class"
unexpectedly found "<asid" when slicing source, but expected "class"
unexpectedly found "<asid" when slicing source, but expected "class"
unexpectedly found "<asid" when slicing source, but expected "class"
unexpectedly found "<asid" when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"
unexpectedly found "<div " when slicing source, but expected "class"

interestingly we are only seeing it with class and I'm wondering if it is related to us using ember-css-modules.

rwjblue commented 3 years ago

@Turbo87 - What is the template snippet that causes that output?

Turbo87 commented 3 years ago

I can't tell because the snippet is not mentioned in the logs, but it appears to be happening for almost all of our templates, judging by the amount of these log lines.

rwjblue commented 3 years ago

I'm slightly confused. Are these actual errors or is it console output during template compilation? If it is just console output (still horrible, don't get me wrong) does the application work properly (e.g. pass tests and what not)?

Turbo87 commented 3 years ago

Are these actual errors or is it console output during template compilation?

just console output during the build steps.

If it is just console output (still horrible, don't get me wrong) does the application work properly (e.g. pass tests and what not)?

yes, everything appears to be working as intended. or at least there is nothing visibly wrong and the tests are passing fine.

since I don't know what is causing this message I can't tell if we're shipping any additional debug code or something like that, but our asset size tracking has not shown any significant increases from updating to 3.25, so that also seems unlikely.

Turbo87 commented 3 years ago

@rwjblue if you need a repro, the crates.io codebase is currently showing these warnings too :)

rwjblue commented 3 years ago

OK, awesome. That is helpful, thank you!

boris-petrov commented 3 years ago

I believe this issue has been resolved in the past weeks/months - I don't seem to see the warnings any more. @Turbo87 - am I right? @rwjblue - any ideas when this was fixed? :)

nelstrom commented 3 years ago

@boris-petrov which version of Ember are you using? We are on ember-source version 3.26.1 and we still get these messages. I'm curious whether upgrading to 3.27.x might silence these messages?

boris-petrov commented 3 years ago

@nelstrom - I am on the latest - 3.27.5. Not sure if that was what resolved the issue though. But the fact is that I haven't seen these warnings in a while now.

nelstrom commented 3 years ago

@boris-petrov ok, that gives me some motivation to upgrade.

boris-petrov commented 2 years ago

I started getting these errors again after updating ember-cli-htmlbars from 6.0.0 to 6.0.1 (as well as a bunch of other ones). So I guess the issue hasn't been resolved after all. Any ideas?

bertdeblock commented 2 years ago

I'm seeing similar logs when updating ember-cli-htmlbars to 6.0.1:

unexpectedly found "<@Subse" when slicing source, but expected "@source"

The @source argument comes from an AST transform in ember-freestyle: https://github.com/chrislopresto/ember-freestyle/blob/master/lib/ast-transform.js#L91

I guess the issue (in my case) has always been there, but just wasn't logged.

Any directions on how I can solve this would be greatly appreciated!

basz commented 2 years ago

I'm noticing a few [Babel: @xxxx/xxxx > applyPatches]unexpectedly found "<div " when slicing source, but expected "class" instances.

not sure if it comes from there but my application and its addons (mono repo with yarn packages) are dependant on ember-cli-htmlbars@^5.7.2

dfreeman commented 2 years ago

I hit this in a repo that uses ember-css-modules the same way @Turbo87 described. It definitely seems plausible it's related to the template transform happening in that package, or from the other comments here from template transforms in general

nruth commented 2 years ago

I'm also seeing it in an app with ember-css-modules after upgrading to 4.2.

I found this in a project search, maybe someone comitted this by mistake (or is this generated?):

node_modules/ember-source/dist/ember-template-compiler.js

Screenshot 2022-03-08 at 02 58 57

This looks different to the following similar code, which I guess is to do with different javascript ecosystems/modules/packagers.

image image
bertdeblock commented 2 years ago

@nruth I think that's just generated code in development, those DEBUG blocks are normally stripped away in production builds if I'm not mistaken.

dfreeman commented 2 years ago

I poked at this a bit today, and it looks like adding any synthetic AttrNode in a template transform will trigger this.

I don't have great in-depth knowledge of the AST normalization process, but these lines seem like they're treating the loc of the synthetic node as a real location that they can read from in source, which is triggering the warning here.

rwjblue commented 2 years ago

Makes sense, thanks for digging into that @dfreeman. I think the next thing to do is to avoid that warning code path when working on synthetic AST nodes...

runspired commented 2 years ago

Also hit this when working to modernize flexi (which produces synthetic Attr nodes). I went down the rabbithole of adjusting loc because it otherwise spits out thousands of these.

colenso commented 1 year ago

Is this an issue with glimmer.js? If yes, will it be fixed in 2.0?

runspired commented 1 year ago

its a bug in the sense that it outputs a meaningless warning, its not actually an issue beyond that

navels commented 1 year ago

This is also impacting consumers of the ember-basic-dropdown addon (which uses ember-maybe-in-element which creates synthetic attributes). I don't really see how to work around this in that addon as ember-css-modules was able to.

I would argue this is well worth fixing (at a minimum: don't output the warning if the node is synthetic, if possible). To developers, this looks like there is a syntax error somewhere. Any warnings that appear during the course of migration are particularly distracting, because we are on the lookout for new problems.

One could require plugins to set the attribute's loc.source:

newAttr.loc = {
  source: '(synthetic)',
  start: { line: 1, column: 0 },
  end: { line: 1, column: 0 }
};

and key off that . . . ?

navels commented 1 year ago

PR for fixing the {{~!}} case: https://github.com/glimmerjs/glimmer-vm/pull/1430

PR for fixing the remaining cases: https://github.com/glimmerjs/glimmer-vm/pull/1431/files

yuriyaran commented 1 year ago

PR for fixing the {{~!}} case: glimmerjs/glimmer-vm#1430

PR for fixing the remaining cases: https://github.com/glimmerjs/glimmer-vm/pull/1431/files

@navels, thanks for the PRs! any plans to make checks pass? @NullVoxPopuli? I'd be happy to have this merged - since console warnings are indeed distracting

NullVoxPopuli commented 1 year ago

We're waiting on some glimmer vm refactoring work atm, at which point, vm fixes very likely won't be back portable, depending on complexity of the change. We'll see what happens once the long overdue maintenance is done. (The linked fix above looks fairly isolated and straight forward tho)

Stay tuned!

navels commented 1 year ago

Thanks, I will stay tuned. Happy to retest and update the PRs if they are still warranted.

KoKuToru commented 1 year ago

Any updates on this? I am adding synthetic AttrNode to every ElementNode and get hundreds of messages in the console, pretty anoying.

Checking the code of https://github.com/glimmerjs/glimmer-vm/blob/master/packages/%40glimmer/syntax/lib/v1/public-builders.ts , it is not possible (or not easy) to set a useful loc that would hide the messages without referencing a existing slice of the original source. Changing the source or overwriting the source is not possible..

navels commented 8 months ago

Any updates on the refactoring that was blocking this fix?

NullVoxPopuli commented 8 months ago

Someone probably needs to do the work in @glimmer/syntax