Allow Fastboot to work with Lazy Loading Engines #444
Conversation
lib/engine-addon.js
Outdated
| // Separate engines routes from the host if includeRoutesInApplication | ||
| // is false | ||
| var separateRoutes = this.lazyLoading.includeRoutesInApplication === false; | ||
| if (separateRoutes) { |
There was a problem hiding this comment.
I believe the tests are failing partially due to the collisions here. Basically, all the old stuff should still be in the addon we just want to add configEnvironment from the new part to the finalMergeTrees, does that make sense?
| assert.deepEqual(loadEvents[0].split(SEPARATORS), [ '', 'engines-dist', 'ember-blog', 'assets', 'engine-vendor.css' ], 'loaded engine vendor css'); | ||
| assert.deepEqual(loadEvents[1].split(SEPARATORS), [ '', 'engines-dist', 'ember-blog', 'assets', 'engine-vendor.js' ], 'loaded engine vendor js'); | ||
| assert.deepEqual(loadEvents[2].split(SEPARATORS), [ '', 'engines-dist', 'ember-blog', 'assets', 'engine.css' ], 'loaded engine css'); | ||
| assert.deepEqual(loadEvents[3].split(SEPARATORS), [ '', 'engines-dist', 'ember-blog', 'assets', 'engine.js' ], 'loaded engine js'); | ||
| assert.deepEqual(loadEvents[4].split(SEPARATORS), [ '', 'engines-dist', 'ember-blog', 'config', 'environment.js' ], 'loaded environment js'); |
There was a problem hiding this comment.
Hmm, looks like this is loading the new config/environment.js file that we use for Node land. This is sub-optimal as it shouldn't be needed in the browser.
Seems like we have a couple options:
- Put the environment config file elsewhere in the output so it doesn't get included in the asset manifest
- Add an option to the asset manifest generator to ignore the environment config (would require changes to
ember-asset-loader) - Add an additional build step to remove the environment config entry from the generated manifest
@rwjblue do you have any preference here? I'm leaning towards option #2.
There was a problem hiding this comment.
What have we decided about this? How do we want to implement it?
There was a problem hiding this comment.
Yes, agreed the second option above seems like the best path forward to me.
(sorry for the delays here)
|
@devotox looks like there are still some test failures; let me know if you'd like some help bringing this over the line (I know this code base is kind of crazy) |
|
@trentmwillis Yes please. The errors seem to be with the fact that there it expects 3 assets but it has 4. I thought the updates with the Ember Asset Loader done by @rwjblue would be enough but I'm guessing there is something more i have to do to exclude the environment config? |
|
@devotox the changes needed in ember-asset-loader haven't been made yet. I just opened a PR for those: ember-engines/ember-asset-loader#58. Once that's merged we can ignore the |
|
I've released [email protected] which enables us to ignore the environment config files. Will try integrating it into this PR soon. |
|
Ive now also noticed that this is an issue causing a problem with the integration
emberjs/ember.js#15615
|
|
Anything more on the integration of ember-asset-loader? |
|
FWIW emberjs/ember.js#15615 was fixed and released in [email protected]. |
|
Hate to bounce to yet another PR, but rebasing this was too difficult. I've cleaned up the commits and opened a new PR at #494, which I plan on finishing today (assuming nothing goes horribly wrong). Closing this, but will update when the other PR is merged as I know multiple folks are following this thread. |
cc: @trentmwillis
This fixes the merge conflicts from @trentmwillis branch and updates the manifest hook to use lazyLoading.enabled flag