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

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

Closed
ztjohns opened this issue Jan 6, 2023 · 10 comments · Fixed by #1381
Closed

Comments

@ztjohns
Copy link

ztjohns commented Jan 6, 2023

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
Copy link

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

@ztjohns
Copy link
Author

ztjohns commented Jan 26, 2023

@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
Copy link
Author

ztjohns commented Jan 26, 2023

@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
Copy link
Collaborator

Windvis commented Jan 26, 2023

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
Copy link
Contributor

ef4 commented Jan 26, 2023

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

@RobbieTheWagner
Copy link

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

@ef4
Copy link
Contributor

ef4 commented Jan 26, 2023

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
Copy link
Collaborator

Windvis commented Mar 24, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

ef4 commented Mar 24, 2023

Yes, the exact fix was here:

// the extra underscore here guarantees that we will never collide with an
// HTML element.
return parts[parts.length - 1] + '_';

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

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 a pull request may close this issue.

5 participants