Add Embroider support #34

simonihmig · 2020-11-17T15:30:03Z

Fixes #30

Embroider has a special dependency rule in place in to support the dynamic component pattern in Embroider, which made this addon work in Embroider for a while. However it seems embroider-build/embroider#608 added some stricter checks for dynamic component usage that again made this addon fail under Embroider.

This should now hopefully solve the issue once and for all, by wrapping the private -element helper in Embroider's ensure-safe-component, which also allows us to get rid of the -dynamic-element-* components (see code comment).

Tested this locally in an app, and added ember-try scenarios.

To my understanding the changes here should be ok, especially the use of the ensure-safe-component helper, but @ef4 if you have the time to give this a review from Embroider's perspective, this would be helpful!

simonihmig · 2020-11-17T15:33:50Z

tests/integration/helpers/element-test.js

@@ -89,7 +89,7 @@ module('Integration | Helper | element', function(hooks) {
        {{#let (element "button") as |Tag|}}\
          <Tag type="button" id="action" {{on "click" this.didClick}}>hello world!</Tag>\
        {{/let}}\
-      ', { insertRuntimeErrors: true }));


I removed this as Embroider seems to have its own hbs babel-plugin, that throws when receiving more than one argument. I am not sure what this option does here, so please advice if/how this needs to be fixed!?

simonihmig · 2020-11-17T15:37:37Z

A few scenarios are failing, but it seems for unrelated reasons. Also the latest scheduled run on main had some failures...

ef4 · 2020-11-17T16:15:35Z

Hmm, if you're able to delete the -dynamic-element components that seems like ensure-safe-component maybe isn't stable enough, and it could be hurting re-render performance.

simonihmig · 2020-11-17T16:24:42Z

Hmm, if you're able to delete the -dynamic-element components that seems like ensure-safe-component maybe isn't stable enough, and it could be hurting re-render performance.

I don't follow you tbh... just to prevent any misunderstandings: the -dynamic-element components are not used anymore in this PR. They are just still present as Embroider's dependency rule that I added a while ago still refers to them, and removing them here at this point in time would make Embroider complain about those missing.

simonihmig · 2020-11-17T16:32:08Z

A few scenarios are failing, but it seems for unrelated reasons.

Hm, not quite (re: unrelated):

yes, classic and jquery also fail on the main branch
beta and canary fail with owner.factoryFor being undefined, the call stack includes ensureSafeComponent. Which has some special workaround for 3.24 (which seems to have been promoted to beta recently), so maybe that is not working correctly? /cc @ef4
3.4 fails with Element modifiers are not allowed in components which I am not sure if/how this is related to my changes 🤔

addon/helpers/-element.js

ef4 · 2020-11-20T16:12:46Z

@simonihmig I confirmed that there's no stability problem in ensure-safe-component, so my fears above were unfounded.

Also, the problem you hit with ember beta is fixed by embroider-build/embroider#620 and will be released soon.

simonihmig · 2020-11-24T22:58:49Z

I updated the Embroider dependencies after its latest release, which makes most scenarios pass.
The still failing ones are:

default-jquery
classic
3.4

The former two are unrelated to this PR, and failing with the same error on main due to set-env not working anymore

The last one is tricky. Seems the second argument ({ insertRuntimeErrors: true }) to hbs mentioned above is there to allow Ember <3.11 to compile the template. While at the same time this second argument makes Embroider throw a compilation error as mentioned.

I already tried replacing ember-compatibility-helpers with @embroider/macros, in the hope that the latter's dead branch elimination would make the offending hbs call be hidden from the template compiler. But it seems the template compiler's transform happens before that of @embroider/macros, so that doesn't help here.

So as it stand either the Ember 3.4 scenario or the two Embroider scenarios added here will fail with a build error. I have no better idea to solve this than to disable the 3.4 scenario!? 🤔

However we end up fixing this, it's certainly "just" a CI issue. Except for this, I believe the changes here should be fine now from my PoV, and looking forward for a final review! (as this is a blocking issue for some addon's embroider progess 😉 )
/cc @chancancode

ef4 · 2020-11-29T22:50:31Z

@simonihmig I think the solution here is that Embroider needs to implement insertRuntimeErrors.

simonihmig · 2020-12-19T15:14:04Z

I think the solution here is that Embroider needs to implement insertRuntimeErrors.

I planned to do that, but time was flying away, and as it stands I am not able to do that in the near future, at least not in the remaining days of this year.

As stated above, the issue is "just" about testability, not the actual code. So is there a chance we can land this without being blocked on the test coverage here (again, the change in the actual code is blocking embroider adoption for some addons, at least ember-bootstrap)? @chancancode

ef4 · 2020-12-19T17:13:19Z

insertRuntimeErrors is now implemented here embroider-build/embroider#658

ef4 · 2020-12-20T20:35:08Z

Embroider 0.35.0 was released with support for insertRuntimeErrors.

simonihmig · 2020-12-20T23:31:52Z

Embroider 0.35.0 was released with support for insertRuntimeErrors.

Awesome, thanks so much! 🙏

I previously replaced ember-compatibility-helpers with @embroider/macros, hoping to fix the insertRuntimeErrors issue by that, which did not work out. But I figured this change is useful by itself, so I pushed that here. Also Ember 3.25 Canary started to throw a different error for that "invalid usages: it does not take a block" test (related to this GlimmerVM update), so I also pushed a commit fixing that.

CI is "green" now (again, 2 failing jobs are broken on main branch also), so ready for a final review please! 🙏

simonihmig · 2021-02-01T22:13:04Z

As the CI failures on the main branch have been fixed meanwhile, I rebased this, and CI is green now!

Hope we can finally get this reviewed and eventually merged, to unblock Embroider adoption in the ecosystem! @chancancode @rwjblue

lib/element-helper-syntax-plugin.js

package.json

chriseppstein · 2021-03-21T18:46:13Z

Is there an ETA on landing this?

simonihmig · 2021-03-22T16:29:33Z

Is there an ETA on landing this?

Only waiting for final approval. @rwjblue I addressed your feedback long ago, could you please have another look?

cibernox · 2021-04-11T21:42:48Z

what's the status of this?

simonihmig · 2021-04-12T08:06:13Z

what's the status of this?

Same as 3 weeks ago:

Only waiting for final approval.

🤷‍♂️

chancancode · 2021-04-12T08:57:19Z

We also needed to fix our embroider build at Skylight so we can look into merging and tagging it this week.

I am not sure I am the best person to catch mistakes relating to embroider itself, other than confirming that it does indeed work in the app. Anything you would like to call out specifically?

Sorry for the wait!

simonihmig · 2021-04-12T09:16:04Z

I am not sure I am the best person to catch mistakes relating to embroider itself, other than confirming that it does indeed work in the app. Anything you would like to call out specifically?

Not really. @ef4 and @rwjblue both did an initial review, and I think I have addressed everything. Also I am using this branch in an Embroider-built app, and this PR adds test coverage for Embroider, so I am pretty confident it does work as it should.

rwjblue

OK, sorry for the long delay again @simonihmig. I think the only thing remaining here is to bump the @embroider packages to latest, then we are good to go.

package.json

…oider

simonihmig · 2021-04-12T16:12:16Z

I think the only thing remaining here is to bump the @embroider packages to latest, then we are good to go.

@rwjblue rebased and updated all embroider dependencies!

rwjblue · 2021-04-12T17:16:05Z

Thanks, sorry again for the delays.

chancancode · 2021-04-12T20:43:31Z

I can tag/publish – is this a minor or major bump?

chancancode · 2021-04-12T20:45:15Z

I guess we are pre-1.0 so I will push a 0.5.0

rwjblue · 2021-04-12T20:46:08Z

@chancancode - It should be fully safe (aka non-breaking)

chancancode · 2021-04-12T20:48:43Z

Oops, pushed 0.5.0 already. I supposed it could have been 0.4.1 but doesn't really matter that much

simonihmig commented Nov 17, 2020

View reviewed changes

ef4 reviewed Nov 17, 2020

View reviewed changes

addon/helpers/-element.js Outdated Show resolved Hide resolved

akatz mentioned this pull request Nov 21, 2020

[Embroider] replace with safe component cibernox/ember-power-select#1419

Closed

This was referenced Nov 21, 2020

Fix ensureSafeComponent to not reuse registered component across owners embroider-build/embroider#624

Merged

Fix optimized Embroider build ember-bootstrap/ember-bootstrap#1341

Merged

GavinJoyce mentioned this pull request Dec 9, 2020

[spike] try @embroider/test-setup GavinJoyce/ember-headlessui#24

Closed

simonihmig force-pushed the embroider branch 2 times, most recently from 10a1c7c to 83c7aeb Compare December 20, 2020 22:05

simonihmig force-pushed the embroider branch from 39fc3ee to 3b5b6dd Compare February 1, 2021 21:52

rwjblue reviewed Feb 4, 2021

View reviewed changes

lib/element-helper-syntax-plugin.js Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

ef4 mentioned this pull request Feb 10, 2021

[Embroider] replace with safe component cibernox/ember-basic-dropdown#587

Merged

ef4 mentioned this pull request Apr 4, 2021

Embroider Compatibility: animated-container's dynamic Tag breaks build ember-animation/ember-animated#257

Open

gilest mentioned this pull request Apr 11, 2021

issue with embroider cibernox/ember-basic-dropdown#600

Closed

rwjblue reviewed Apr 12, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

simonihmig added 9 commits April 12, 2021 17:54

Setup embroider tests

39b8111

Wrap dynamic component usage w/ ensure-safe-component to support Embr…

c937456

…oider

(Re-)Use alternating component classes

bca717c

Update Embroider dependencies

d49b47e

Replace ember-compatibility-helpers with @embroider/macros

7028631

Update @embroider/util to fix issues on Ember Canary

1642018

Fix prerelease versions

149b758

Revert Ember downgrade

fb3c407

Update embroider deps

d447df3

simonihmig force-pushed the embroider branch from 5330e07 to d447df3 Compare April 12, 2021 15:55

rwjblue approved these changes Apr 12, 2021

View reviewed changes

rwjblue merged commit 334b844 into tildeio:main Apr 12, 2021

simonihmig mentioned this pull request Apr 13, 2021

Release notes for v0.4.0 missing? #43

Closed

This was referenced Apr 22, 2021

Added embroider-safe to ember-try scenarios ijlee2/ember-container-query#79

Merged

Updated ember-element-helper to v0.5.0 ijlee2/ember-container-query#80

Merged

rwjblue mentioned this pull request Jun 17, 2021

Regression: unable to parallelize plugin #61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Embroider support #34

Add Embroider support #34

simonihmig commented Nov 17, 2020

simonihmig Nov 17, 2020

simonihmig commented Nov 17, 2020

ef4 commented Nov 17, 2020

simonihmig commented Nov 17, 2020

simonihmig commented Nov 17, 2020

ef4 commented Nov 20, 2020

simonihmig commented Nov 24, 2020

ef4 commented Nov 29, 2020

simonihmig commented Dec 19, 2020

ef4 commented Dec 19, 2020

ef4 commented Dec 20, 2020

simonihmig commented Dec 20, 2020

simonihmig commented Feb 1, 2021

chriseppstein commented Mar 21, 2021

simonihmig commented Mar 22, 2021

cibernox commented Apr 11, 2021

simonihmig commented Apr 12, 2021

chancancode commented Apr 12, 2021

simonihmig commented Apr 12, 2021

rwjblue left a comment

simonihmig commented Apr 12, 2021

rwjblue commented Apr 12, 2021

chancancode commented Apr 12, 2021

chancancode commented Apr 12, 2021

rwjblue commented Apr 12, 2021

chancancode commented Apr 12, 2021

Add Embroider support #34

Add Embroider support #34

Conversation

simonihmig commented Nov 17, 2020

simonihmig Nov 17, 2020

Choose a reason for hiding this comment

simonihmig commented Nov 17, 2020

ef4 commented Nov 17, 2020

simonihmig commented Nov 17, 2020

simonihmig commented Nov 17, 2020

ef4 commented Nov 20, 2020

simonihmig commented Nov 24, 2020

ef4 commented Nov 29, 2020

simonihmig commented Dec 19, 2020

ef4 commented Dec 19, 2020

ef4 commented Dec 20, 2020

simonihmig commented Dec 20, 2020

simonihmig commented Feb 1, 2021

chriseppstein commented Mar 21, 2021

simonihmig commented Mar 22, 2021

cibernox commented Apr 11, 2021

simonihmig commented Apr 12, 2021

chancancode commented Apr 12, 2021

simonihmig commented Apr 12, 2021

rwjblue left a comment

Choose a reason for hiding this comment

simonihmig commented Apr 12, 2021

rwjblue commented Apr 12, 2021

chancancode commented Apr 12, 2021

chancancode commented Apr 12, 2021

rwjblue commented Apr 12, 2021

chancancode commented Apr 12, 2021