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.

Sign up for GitHub

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

Jump to bottom

Does not work with (lazy-loaded) engines #47

Closed
buschtoens opened this issue Sep 24, 2018 · 4 comments · Fixed by #91 · May be fixed by #48
Closed

Does not work with (lazy-loaded) engines #47

buschtoens opened this issue Sep 24, 2018 · 4 comments · Fixed by #91 · May be fixed by #48

Comments

@buschtoens
Copy link

Only tested this with a lazy-loaded engine, but I would assume that eagerly bundled engines are also affected.

Since the runtime polyfill only patches the buildRegistry method of the Application class, it has no effect on engines:

https://github.com/rwjblue/ember-angle-bracket-invocation-polyfill/blob/53afc725e13d8db63bf8006c0c906969441482de/vendor/angle-bracket-invocation-polyfill/runtime-polyfill.js#L57-L60

Which is why it causes this error:

Uncaught Error: Assertion Failed: You cannot use 'SomeComponent' as a component name. Component names must contain a hyphen

I understand that Application extends Engine, but that this polyfill has to patch Application instead of Engine, because it needs to override some registry entries only defined in Application:

https://github.com/emberjs/ember.js/blob/v3.4.1/packages/%40ember/application/lib/application.js#L1091-L1093

I now tried running the same polyfill for Engine in addition to Application and so far it seems to work. 🤷‍♂️

This is not ideal for Application though, because the polyfill is:

  • registered in Engine
  • then overridden in Application
  • and then registered again in Application

Do you still think that this is a valid solution going forward? I will submit a PR then.
@rwjblue
Copy link
Member

rwjblue commented Sep 24, 2018

Definitely seems like a reasonable start, happy to help tweak it in a PR...

@nattydodd nattydodd mentioned this issue Sep 25, 2018
Open
@jbailey4
Copy link

jbailey4 commented Feb 4, 2019 •
edited
Loading

My team recently ran into this as well when creating a new engine within our older application at ember@2.18.2. We wanted to install this as a way to start using some of the newer ember syntax, since the engine would be a greenfield project.

Happy to help with anything needed to get this fix in!

@rwjblue
Copy link
Member

rwjblue commented Apr 8, 2019

#48 seems to have stalled a bit, any folks looking for this functionality willing to pick it up?

@pgengler pgengler mentioned this issue Mar 3, 2020
Merged
@pgengler
Copy link
Contributor

pgengler commented Mar 4, 2020

I opened a new PR (#91) that's just the changes from #48 applied to current master. Tried it out in an Ember 3.1 app that uses engines and it seems to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@rwjblue @pgengler @buschtoens @jbailey4