embroider-build / embroider

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

{{div}} divide helper from ember-math-helpers breaks with 2.x with use of <div> tags #1320

Closed ztjohns closed 1 year ago

ztjohns commented 1 year ago

Hello, when using ember-math-helpers in templates after turning on Embroider 2.0.2 we get the following errors from components that use this divide helper.

Attempted to load a component, but there wasn't a component manager associated with the definition. The definition was: Wrapper

Example

{{#let (div this.a this.b) as |c|}}
  <div>{{c}}</div>
{{/let}}

Temp Solution Fortunately we only used this in two places, so I am moving all the computation in JS instead.

Expectation It to work as normal

RobbieTheWagner commented 1 year ago

@ztjohns can you please confirm if using {{div this.a this.b}} outside of #let gives you the same error?

ztjohns commented 1 year ago

@ztjohns can you please confirm if using {{div this.a this.b}} outside of #let gives you the same error?

Sure thing, let me do a few examples and I will post something.

ztjohns commented 1 year ago

@ztjohns can you please confirm if using {{div this.a this.b}} outside of #let gives you the same error?

The following scenarios in addition to the let example above all cause the wrapper error mentioned:

<div>{{div 10 2}}</div>
<div>hello world</div>
{{div 10 2}}

It seems like simply using the {{div}} helper in a template that has <div> tags fails to parse corrrectly.

Windvis commented 1 year ago

I think the issue is that, starting with @embroider/core v2, components, helpers and modifiers will be automatically added to the template scope. Since your code uses the div helper, Embroider will add it to the scope. This has the side effect that the div in <div> is the same reference, but since it's not a component you'll see the mentioned error message.

When using the first-class templates syntax it would look similar to this:

import div from 'ember-math-helpers/helpers/div';

<template>
  {{#let (div @a @b) as |c|}}
    <div>{{c}}</div>
  {{/let}}
</template>

This version would have the same issue, with the exception that you can simply rename the helper import so it doesn't conflict with the element name anymore.

import divide from 'ember-math-helpers/helpers/div';
...

So I think the issue is that Embroider adds references into the template scope while they might conflict with native element names. Before strict mode templates it wasn't really possible to get into this situation since all references would be prefixed with this.div or @div and there is also the no-shadowed-element template lint rule to prevent some other cases.

ef4 commented 1 year ago

I think @Windvis is right and we need to check for collisions with native events in lexical scope.

RobbieTheWagner commented 1 year ago

I could also easily rename this helper in ember-math-helpers, which might be a good thing to do.

ef4 commented 1 year ago

Yeah, we should fix the bug but renaming the helper is also good, because even if embroider automatically avoids the collision, users writing in first-class template syntax would still need to remember to avoid the collision.

Windvis commented 1 year ago

I was recently testing Embroider in one of our apps and encountered a similar case with the await helper from ember-promise-helpers. In this case it triggered a build error since await is a reserved keyword.

I re-exported the helper under another name in our app and used the updated name to work around the issue.

(just documenting the issue here since other people might run into it as well)

void-mAlex commented 1 year ago

this class of issues should be fixed on latest unstable npm releases, we're also working on adding tests specifically for this type of case, I'll make sure to add await to the list thanks for the report

ef4 commented 1 year ago

Yes, the exact fix was here: https://github.com/embroider-build/embroider/blob/e7c17891eef96ab35f75e7323ea39dc69a36a8b6/packages/compat/src/resolver-transform.ts#L620-L622

(And while the comment says it avoids collision with html elements, this also necessarily avoids collision with javascript keywords like await too.)