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

MU addons must generate a MU dummy app #7667

Merged
merged 1 commit into from Mar 12, 2018

Conversation

Projects
None yet
4 participants
@cibernox
Copy link
Contributor

cibernox commented Mar 7, 2018

module.exports = {
browsers
browsers: [

This comment has been minimized.

@cibernox

cibernox Mar 7, 2018

Contributor

Looking at this diff I've realized that the module-unification-app blueprint is outdate compared with the regular app blueprint. We need to come up with a system that reuses as much as possible from the standard blueprint and only swaps the indispensable files, so the chances of this happening again is lower.

This comment has been minimized.

@ro0gr

ro0gr Mar 7, 2018

Contributor

module-unification-app blueprint is outdate compared with the regular app blueprint

#7595 aims to address this by re-using the same config/targets.js fixture for both app and module-unification-app blueprints testing.

Also I've been working on reducing duplication between these blueprints(#7500) but it was blocked due to bad testing story(I believe).

@cibernox cibernox force-pushed the cibernox:generate-a-mu-dummy-app-for-mu-addongs branch from dffbc98 to d3f60e9 Mar 9, 2018

@@ -40,13 +40,12 @@
"ember-load-initializers": "^1.0.0",
"ember-maybe-import-regenerator": "^0.1.6",
"ember-resolver": "^4.0.0",
"ember-source": "~3.1.0-beta.1",
"ember-source": "http://builds.emberjs.com/canary.tgz",

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

This URL is out of date. I don't think we should ever be using it, /cc @rwjblue yes?

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

Also at

"ember-source": "http://builds.emberjs.com/canary.tgz<% if (welcome) { %>",
for MU apps.

This comment has been minimized.

@rwjblue

rwjblue Mar 11, 2018

Contributor

Confirm. We need to fetch the current URL each time (via ember-source-channel-url which returns a promise)

This comment has been minimized.

@cibernox

cibernox Mar 11, 2018

Contributor

@rwjblue Any guidance how to get the ember-source-channel-url at build time? Specially about how to handle it in testing, where we shouldn't rely on the network.

@@ -0,0 +1,13 @@
import Resolver from 'ember-resolver/resolvers/fallback';

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

I suggest we should have this use the glimmer wrapper instead of the fallback resolver:

import Resolver from 'ember-resolver/resolvers/glimmer-wrapper';

The glimmer wrapper only implements module unification rules. The fallback is only needed if you have classic addons.

This comment has been minimized.

@rwjblue

rwjblue Mar 11, 2018

Contributor

I think all apps have classic addons (literally all addons are “classic” right now).

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

The dummy app of an brand new module unification addon should not have any classic addons.

This comment has been minimized.

@rwjblue

rwjblue Mar 11, 2018

Contributor

Definitely seems ideal, but I highly doubt it is the current state.

This comment has been minimized.

@cibernox

cibernox Mar 11, 2018

Contributor

What is the consensus here?

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

Ok! We had some more discussion. Lets stick with the fallback resolver. We should use the fallback setup as described at https://github.com/rwjblue/ember-module-migrator#running-module-unification-with-fallback-to-classic-app-layout. We want to use this setup for MU apps since there are still classic addons in the default package.json and it is reasonable to expect they work out of the box.

I suggest that we should remove the fallback setup before we remove any feature flags here. I've added that step to our blocker list for removing the feature flags.

This comment has been minimized.

@cibernox

cibernox Mar 12, 2018

Contributor

Added!

Resolver
});

loadInitializers(App, config.modulePrefix + "/src/init");

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

Per the fallback setup instructions at https://github.com/rwjblue/ember-module-migrator#running-module-unification-with-fallback-to-classic-app-layout another loadInitializers line is needed here that loads addon initializers in the app folder.

This comment has been minimized.

@mixonic

mixonic Mar 11, 2018

Contributor

This same addition is needed at

loadInitializers(App, config.modulePrefix + "/src/init");

This comment has been minimized.

@cibernox

cibernox Mar 12, 2018

Contributor

Added

@mixonic

This comment has been minimized.

@cibernox cibernox force-pushed the cibernox:generate-a-mu-dummy-app-for-mu-addongs branch from d3f60e9 to 49d0b16 Mar 12, 2018

@cibernox

This comment has been minimized.

Copy link
Contributor

cibernox commented Mar 12, 2018

This is ready for a second review

@mixonic

This comment has been minimized.

Copy link
Contributor

mixonic commented Mar 12, 2018

@cibernox yesterday @rwjblue was eyeing the ember-source-channel-url stuff. He acknowledged it was non-trivial ;-)

I'm going to merge this as a good incremental improvement but we do need the ember-source-channel-url stuff for it to really work. The ember-canary.tgz file is hopelessly out of date.

@mixonic mixonic merged commit 92a5882 into ember-cli:master Mar 12, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 92.536%
Details
@mixonic

This comment has been minimized.

Copy link
Contributor

mixonic commented Mar 12, 2018

Thank you @cibernox!!

@cibernox cibernox deleted the cibernox:generate-a-mu-dummy-app-for-mu-addongs branch Mar 12, 2018

@mixonic mixonic referenced this pull request Mar 13, 2018

Open

[QUEST] Module Unification: Final Cut #16373

10 of 22 tasks complete

@Turbo87 Turbo87 referenced this pull request Mar 23, 2018

Merged

Ember 3.1 Release Blog Post #3230

1 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment