embroider-build / embroider

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

Support level for `ember-engines` ecosystem #996

Closed villander closed 4 months ago

villander commented 3 years ago

I have an add-on that uses ember-engines and provides the router service for their ecosystem.

I’ve just updated my addon with “ember-cli-update” and I’m getting errors on CI related with embroider safe and optimized scenarios on ember-try.

Build Error (PackagerRunner) in tests/test-helper.js
359
360Module not found: Error: Can't resolve '../config/asset-manifest' in '$TMPDIR/embroider/af7626/tests/dummy/tests/test-helper.js'

How can we solve that? It seems an incompatibility with ember-asset-loader on /config/asset-manifest

You can see more details about CI failing here - https://github.com/villander/ember-engines-router-service/pull/44/checks?check_run_id=3783031395

cc @stefanpenner @ef4 @rwjblue

ef4 commented 3 years ago

Embroider ships with a default compat adapter for ember-asset-loader that is supposed to fix this. Possibly something has changed upstream in ember-asset-loader that requires a corresponding update to the adapter.

villander commented 3 years ago

where is located this default compat adapter that you mentioned @ef4 I'd like to jump into that and debug.

simonihmig commented 3 years ago

@villander you can find them here: https://github.com/embroider-build/embroider/tree/master/packages/compat/src/compat-adapters

villander commented 2 years ago

Thanks, @simonihmig! I'll have a look at that.

toddjordan commented 4 months ago

Was this ever resolved or worked around? Currently running into the same thing on engines when enabling embroider-safe try scenarios, and this thread is left on a cliffhanger ;-)

void-mAlex commented 4 months ago

Was this ever resolved or worked around? Currently running into the same thing on engines when enabling embroider-safe try scenarios, and this thread is left on a cliffhanger ;-)

short answer you don't need asset-manifest with embroider and ember-engines is in the process of adding support for ember source v5 https://github.com/ember-engines/ember-engines/pull/855

embroider already fully supports ember-engines even on most optimized settings

villander commented 4 months ago

@toddjordan I haven't jumped it yet.

@void-mAlex I'm curious, why we're still running this issue if embroider fully supports it?

void-mAlex commented 4 months ago

@void-mAlex I'm curious, why we're still running this issue if embroider fully supports it?

mostly because of bad instructions on what to do from ember engines and a severe lack of maintanance on ember-engines side

prefetching assets is a hack that should not be done in the first place, if you're adopting embroider you can safely delete that code but at the same time you need to follow instructions on embroider side to adopt @embroider/router which will handle these things for you. at the same time embroider will adjust the build so asset-manifest is no longer part of the build which is done via the compat adapter as it shouldn't be there anymore as ember-engines is no longer in charge of isolating the engines code from the rest of the app, that happens in the stage 3 bundler (which ember-engines was partially implementing before we had off the shelf tools to do it for us - also reason why we can just delete a bunch of code)

void-mAlex commented 4 months ago

@villander the solution to this issue can be seen here https://github.com/ember-engines/ember-engines/pull/855/files#diff-62cd483f466ed3d468f40869d51b61285859085d5fdb26ca46b6889056b3fcd5R31-R38 ember-engines is an addon that uses try scenarios for embroider and supporting classic pipeline as well and managing that through the use of macros

toddjordan commented 4 months ago

Can confirm that using macros like the example above gets me past the asset-manifest error in my "embroider-safe" try scenario 👍. Sounds like when I move this addon to full embroider build I can remove the pre-fetch/asset-manifest code and use embroider/router.

void-mAlex commented 4 months ago

closing this as it now has actionable steps

toddjordan commented 4 months ago

I will mention that now that my test suite runs, I'm getting "Attempted to resolve [component or helper name], which was expected to be a [component|helper], but nothing was found". the engine seems to have trouble resolving components and helpers from addon dependencies. Not sure if it has anything to do with this modified test setup for the engine. Am investigating, but thought I'd mention it here in case it is related to the workaround.

void-mAlex commented 4 months ago

@toddjordan is the test for the engine itself? depending on what you're testing you may need to do something like this in the dummy app https://github.com/ember-engines/ember-engines/pull/855/commits/5081bffe4baa80ddf1d4229c8435f83694aab724#diff-e68c2af2455acff9ec0822e4f70339add10cb98613f77cd525f3e9fb141b01fd

toddjordan commented 4 months ago

Yeah, these are engine tests. The components and helpers it can't resolve are in addon dependencies of the engine (that work pre-embroider). I'll give re-exporting them in the dummy app a shot. 👍

villander commented 4 months ago

https://github.com/embroider-build/embroider/issues/996#issuecomment-2137099509

Thank you so much @void-mAlex