embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
331 stars 137 forks source link

hbs-loader incompatible with components named `Link` #1176

Open runspired opened 2 years ago

runspired commented 2 years ago

Example error:

ERROR in ../../engines/labor-employee-engine/components/shift-day.hbs
Module Error (from ../../../../../../../../../../../Users/runspired/github/private/fnb/node_modules/@embroider/hbs-loader/src/index.js):
(Emitted value instead of an instance of Error) SyntaxError: Closing tag </div> did not match last open tag <Link> (on line 24): 

The issue here I think is that something the loader is using is confusing <Link></Link> for the element <link />. Capitalization needs to be taken into account.

NullVoxPopuli commented 2 years ago

I have this same problem in my work's codebase, and is one reason why embroider migration has been paused in favor of v2 addon migration :upside_down_face:

ef4 commented 2 years ago

I can't reproduce this. Please try to make a shareable reproduction.

I suspect some addon is installing an AST transform that misbehaves under embroider.

NullVoxPopuli commented 2 years ago

@runspired do you have ember-css-modules installed? Guess it's time to start comparing our deps :sweat_smile:

runspired commented 2 years ago

@NullVoxPopuli I do not, but the addon that is causing this is an engine 🤔

runspired commented 2 years ago

@ef4 I think your intuition is half-right. It only reproduces if I include any template preprocessor, even one that does literally nothing. Here's a repro of that: https://github.com/runspired/link-error-demo

runspired commented 2 years ago

I spent a little time tinkering with different component names, this problem specifically occurs when a template preprocessor is present and the lower-case of a component name collides with an always-self-closing html element (link for example). So a component named Input or Script or A is fine, but one named Hr or Br or in this case Link is not.

ef4 commented 2 years ago

Thanks. Your reproduction let me narrow it down to a bug in the glimmer AST printer. See https://github.com/glimmerjs/glimmer-vm/pull/1388

That PR is just a failing test case. If somebody wants to have a go, the failing test makes it pretty clear if you've succeeded.

ef4 commented 2 years ago

This matters for Embroider because we rewrite all v1-formatted addons to v2-format (so that subsequent build stages can be in a pure v2 world and be much simpler). One thing we do during that rewriting is run any custom transforms. That means that templates get parsed and re-printed. So a bugs in the printer effect us in a way that doesn't matter in classic builds.

This bug only happens in v1 addons. In app code, it shouldn't occur. And in v2 addons it shouldn't occur. It's only the compat layer for v1 addons that relies on the glimmer AST printer.

ef4 commented 2 years ago

I think this bug might apply to all these names:

https://github.com/glimmerjs/glimmer-vm/blob/07db0655c8d4b5c71792bf49ba3b547c196e460a/packages/%40glimmer/syntax/lib/generation/printer.ts#L9

based off investigation by @Windvis.