Add `module-unification-app` blueprint by GavinJoyce · Pull Request #7488 · ember-cli/ember-cli

GavinJoyce · 2017-12-06T14:49:43Z

This is the first step in a series of PRs which will bring support for Module Unification blueprints and generators.

An app using the module unification layout can now be created as follows:

MODULE_UNIFICATION=true ember new my-app
MODULE_UNIFICATION=true ember new my-app --yarn

Once this lands, https://github.com/emberjs/ember-module-unification-blueprint can be removed

TODO:

add module unification app blueprint (extracted from https://github.com/emberjs/ember-module-unification-blueprint)
tests
address feedback

TODO in future PRs:

support init command
support generate commands
support addons
reduce the duplication between classic and MU blueprints (see Add module-unification-app blueprint #7488 (comment))
remove bower and user ember canary in package.json
replace {{welcome-page}} addon with component example

GavinJoyce · 2017-12-06T16:20:56Z

lib/commands/new.js

  run(commandOptions, rawArgs) {
+    if (process.env.MODULE_UNIFICATION === 'true' && commandOptions.blueprint === 'app') {
+      commandOptions.blueprint = 'module-unification-app';
+    }


I'll extract some of this in a subsequent PR once I've got a second place that needs to use it

rwjblue · 2017-12-06T17:05:37Z

support addons

@mansona and I have a spike of what the output for addons should look like in https://github.com/stonecircle/ember-addon-output/tree/feature/module-unification-initial-work, hopefully that will help with adding addon support with the env flag...

rwjblue

Very exciting!

As I am reviewing, I am getting concerned about maintenance of both blueprints over time with the structure here in the PR. Do you think we could refactor things a bit such that we only have to maintain the differences from the main app blueprint? I think that would dramatically reduce the total maintenance burden if we can swing it.

@GavinJoyce - What do you think?

rwjblue · 2017-12-06T17:06:46Z

blueprints/module-unification-app/files/bower.json

@@ -0,0 +1,6 @@
+{


We don't emit bower.json any longer

Hmm, maybe we still need one to get ember-canary build? 🤔

LMK if it's possible to opt in to canary in package.json. Otherwise, I'll leave this as is for now

We can (via github:emberjs/ember.js) but it requires npm@5 or yarn@1 I believe. Seems good to test if you have time?

I couldn't seem to get this to work. I'll add to this my list and come back to it in a future PR

rwjblue · 2017-12-06T17:07:12Z

blueprints/module-unification-app/files/.eslintrc.js

@@ -0,0 +1,13 @@
+module.exports = {


Should be sync'ed with app's .eslintrc.js file (we use overrides and such now)...

rwjblue · 2017-12-06T17:08:50Z

blueprints/module-unification-app/files/public/crossdomain.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0"?>


I think this file is removed for the normal app blueprint?

rwjblue · 2017-12-06T17:09:31Z

blueprints/module-unification-app/files/tests/.eslintrc.js

@@ -0,0 +1,5 @@
+module.exports = {


The usage of overrides in .eslintrc.js should remove the need for these files

rwjblue · 2017-12-06T17:12:09Z

blueprints/module-unification-app/files/src/ui/components/my-component/component.js

@@ -0,0 +1,5 @@
+import Component from "@ember/component";


We probably don't want to emit a component...

rwjblue · 2017-12-06T17:12:22Z

blueprints/module-unification-app/files/src/ui/components/my-component/template.hbs

@@ -0,0 +1,5 @@
+<code>src/ui/components/my-component/template.hbs</code>


rwjblue · 2017-12-06T17:12:41Z

blueprints/module-unification-app/files/src/ui/routes/index/template.hbs

@@ -0,0 +1,3 @@
+<code>src/ui/routes/index/template.hbs</code>
+
+{{my-component}}


Should invoke the {{welcome-page}} addon...

To mirror the app blueprint, I'm going to zap the index route and add {{welcome-page}} to application.hbs

rwjblue · 2017-12-06T17:14:04Z

tests/acceptance/new-test.js

+    ]);
+
+    confirmBlueprintedForDir('blueprints/module-unification-app');
+    process.env.MODULE_UNIFICATION = undefined;


Should probably done in an afterEach in case the generation blows up and leaves us in a bad state...

GavinJoyce · 2017-12-06T17:24:46Z

Thanks for the feedback @rwjblue. I copied over the content of https://github.com/emberjs/ember-module-unification-blueprint directly and didn't consider making any changes to the content of those files. I'll address your feedback this evening, thanks

GavinJoyce · 2017-12-06T17:33:56Z

I am getting concerned about maintenance of both blueprints over time with the structure here in the PR. Do you think we could refactor things a bit such that we only have to maintain the differences from the main app blueprint? I think that would dramatically reduce the total maintenance burden if we can swing it.

@rwjblue That's interesting, I'll have a think about how we might solve this. Do we know how long we expect to have to maintain the two blueprints for? Are we sure that the pain caused by having to maintain duplicate blueprints outweighs the additional complexity and possible brittleness that would come with a solution to share commonality?

GavinJoyce · 2017-12-06T20:24:04Z

A possible solution would be to define the shared files in one path and the specific files in another, something like:

blueprints/shared/app
blueprints/app
blueprints/module-unification-app

The app blueprint would be the merge of blueprints/shared/app & blueprints/app. The module-unification-app blueprint would be the merge of blueprints/shared/app & blueprints/module-unification-app.

We'd need to make some changes to how we test too.

@rwjblue any thoughts on this approach?

rwjblue · 2017-12-07T11:49:19Z

In general, it seems good to me.

FWIW, I do agree with your earlier point too (around adding complexity for a temp situation), I’m just not sure how to guess how long we will need to support both. I suspect it will be at least a couple of releases...

I’m mostly just trying to set us up for success and I worry that during the time where we maintain both we might accidentally forget to keep them in sync properly and therefore make the end-user tester experience much worse...

GavinJoyce · 2017-12-07T11:54:36Z

How about we live with the duplication for the short term and build out support for module unification app, addon and generators first. Once we have that, we'll have a better picture of the overlap and we can then make a decision to either to begin work on removing the duplication or to defer the work until a little later (or perhaps never).

This will allow me to initially focus on landing the feature for MU support in blueprints

rwjblue · 2017-12-07T11:56:42Z

Ok, sounds good to me.

GavinJoyce · 2017-12-07T13:49:53Z

blueprints/module-unification-app/files/package.json

+    "ember-welcome-page": "^3.0.0<% } %>",
+    "eslint-plugin-ember": "^5.0.0",
+    "loader.js": "^4.2.3"
+  },


I've updated these dependencies to more closely mirror https://github.com/ember-cli/ember-cli/blob/d9c015974436ab0b80f552ed7689e968e3d50f91/blueprints/app/files/package.json

GavinJoyce · 2017-12-07T14:03:05Z

@rwjblue I've addressed your feedback.

Here's a repo with the current output of MODULE_UNIFICATION=true ember new module-unification-blueprint-app-output:

https://github.com/GavinJoyce/module-unification-blueprint-app-output

rwjblue · 2017-12-07T14:45:51Z

blueprints/module-unification-app/files/src/resolver.js

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


Changing this to ember-resolver/resolvers/fallback should allow "classic" addons to continue to work within module unification apps (and therefore fix {{welcome-page}} without modification). Can you test to confirm?

Thanks, I've made the change and confirmed that this works

rwjblue

Left some more comments (sorry for the back and forth), also I think we need to add a bower install step to .travis.yml for CI to work properly (since at this point the blueprint still uses bower.json). Mind configuring your module-unification-app-output repo up with Travis and making sure we get a passing run?

rwjblue · 2017-12-07T14:54:09Z

blueprints/module-unification-app/files/tests/helpers/resolver.js

@@ -0,0 +1,11 @@
+import Resolver from '../../src/resolver';


This file should be removed

rwjblue · 2017-12-07T14:54:39Z

blueprints/module-unification-app/files/tests/test-helper.js

+} from 'ember-qunit';
+import { start } from 'ember-cli-qunit';
+
+setResolver(resolver);


This should be setApplication instead (see app blueprint for example)

rwjblue · 2017-12-07T15:00:24Z

blueprints/module-unification-app/files/config/environment.js

@@ -0,0 +1,57 @@
+/* eslint-env node */


this comment should be removed

rwjblue · 2017-12-07T15:00:35Z

blueprints/module-unification-app/files/config/targets.js

@@ -0,0 +1,9 @@
+/* eslint-env node */


this comment is no longer needed with overrides config

rwjblue · 2017-12-07T15:00:43Z

blueprints/module-unification-app/files/ember-cli-build.js

@@ -0,0 +1,25 @@
+/* eslint-env node */


rwjblue · 2017-12-07T15:00:53Z

blueprints/module-unification-app/files/testem.js

@@ -0,0 +1,19 @@
+/* eslint-env node */


rwjblue · 2017-12-07T15:01:34Z

blueprints/module-unification-app/files/tests/test-helper.js

@@ -0,0 +1,8 @@
+import resolver from './helpers/resolver';
+import {
+  setResolver


setApplication

GavinJoyce · 2017-12-07T21:30:39Z

@rwjblue I've addressed your feedback in f8157a5 (I'll squash when we're happy).

I've tested the output with and without yarn and all looks good:

MODULE_UNIFICATION=true .ember new module-unification-blueprint-app-output

https://travis-ci.org/GavinJoyce/module-unification-blueprint-app-output (🍏 )
https://github.com/GavinJoyce/module-unification-blueprint-app-output

MODULE_UNIFICATION=true .ember new module-unification-blueprint-app-yarn-output --yarn

https://travis-ci.org/GavinJoyce/module-unification-blueprint-app-yarn-output (🍏 )
https://github.com/GavinJoyce/module-unification-blueprint-app-yarn-output

ro0gr · 2017-12-12T19:39:10Z

@GavinJoyce looks like "blueprints/module-unification-app/files/yarn.lock" added by mistake(couldn't find a way to inline comment in empty file)

ro0gr · 2017-12-12T19:55:49Z

Do you think we could refactor things a bit such that we only have to maintain the differences from the main app blueprint? I think that would dramatically reduce the total maintenance burden if we can swing it.

We already have a nice example for sharing files between app and addon blueprints.
I think if we share at least files root files(.eslintrc, .editorconfig, testem.js, etc) it would be a win.

I'll take a look in this direction and report back.

ro0gr · 2017-12-12T22:21:16Z

I've done some experimentation to reduce amount of files to maintain #7500
Implementation still can be improved but it looks like addon blueprint's approach works for MU blueprint as well.

ro0gr · 2017-12-12T22:32:10Z

blueprints/module-unification-app/index.js

+
+  fileMapTokens(options) {
+    return {
+      __component__() { return options.locals.component; },


I can't see component defined in a locals. Do we need this?

GavinJoyce · 2017-12-13T09:27:12Z

I've done some experimentation to reduce amount of files to maintain #7500
Implementation still can be improved but it looks like addon blueprint's approach works for MU blueprint as well.

@ro0gr nice one, this will make sharing files across blueprints easier (we'll still need to change how we test the output, as you noted in your PR). I'm still not clear which is better, 1) maintaining two separate trees and living with duplication for a relatively short amount of time or 2) using your technique with this.lookupBlueprint('app').files(); to share a common base. I think it would be good to continue to use 1) in the short term and wait for some data before making an informed decision to jump to 2).

ro0gr · 2017-12-13T10:46:55Z

I'm still not clear which is better, 1) maintaining two separate trees and living with duplication for a relatively short amount of time or 2) using your technique with this.lookupBlueprint('app').files(); to share a common base

@GavinJoyce I'm not sure how short is the "short term" you refer to. But it's pretty obvious that duplication of large amount of files rarely leads to a good consequences. It's quite complicated to sync changes in +24 files(that's an amount of files we drop if share app blueprint files).

I think even now we might have syncing issues. Here is the list of files that are different between app and module-unification-app blueprints:

I'm not sure if that is intentional but it's much more convenient to have only those parts which are actually different even in a short term.

So I would vote to consider sharing files at least because technically it's the same approach we already use for addon blueprint, it seems to work and I can't see any obvious drawbacks with this approach.

mixonic · 2017-12-18T17:48:31Z

blueprints/module-unification-app/files/src/resolver.js

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


I'd very much like to have the default generated app not use the fallback resolver. IMO new apps should default to the future/optimal mode, and if you want to use a legacy addon or app/ directory you can opt-in to the fallback.

mixonic · 2017-12-18T17:49:47Z

Landing this and let's continue to improve it. Both @GavinJoyce and @ro0gr are interested in keeping the iteration moving.

cibernox · 2017-12-24T18:40:26Z

Probably this is a known issue, but once inside an app created with the new blueprint, running ember g route foo fails.
It creates a couple files in /app/routes and then fails with ENOENT: no such file or directory, open '<app-root>/app/router.js'

I also tried adding MODULE_UNIFICATION=true but it only seems to work with the app blueprint.

rwjblue · 2017-12-24T19:20:00Z

I believe we will need to update those blueprints in ember-source itself.

GavinJoyce force-pushed the gj/module-unification-blueprints branch from 27e3a4b to 6c3406a Compare December 6, 2017 16:15

GavinJoyce changed the title ~~[WIP] add module-unification-app blueprint~~ Add module-unification-app blueprint Dec 6, 2017

GavinJoyce commented Dec 6, 2017

View reviewed changes

rwjblue reviewed Dec 6, 2017

View reviewed changes

GavinJoyce force-pushed the gj/module-unification-blueprints branch 2 times, most recently from 9ee97dd to 27cf0f3 Compare December 7, 2017 13:46

GavinJoyce commented Dec 7, 2017

View reviewed changes

rwjblue reviewed Dec 7, 2017

View reviewed changes

GavinJoyce force-pushed the gj/module-unification-blueprints branch from 27cf0f3 to cab2862 Compare December 7, 2017 14:48

rwjblue reviewed Dec 7, 2017

View reviewed changes

ro0gr mentioned this pull request Dec 12, 2017

Put MU blueprint on a diet #7500

Closed

ro0gr reviewed Dec 12, 2017

View reviewed changes

[FEATURE] add module-unification-app blueprint

4a0c774

GavinJoyce force-pushed the gj/module-unification-blueprints branch from 9202add to 4a0c774 Compare December 18, 2017 09:15

mixonic reviewed Dec 18, 2017

View reviewed changes

mixonic merged commit 80b4a7a into ember-cli:master Dec 18, 2017

GavinJoyce deleted the gj/module-unification-blueprints branch December 18, 2017 17:56

ro0gr mentioned this pull request Dec 20, 2017

Add lint:js scripts. #7452

Merged

This was referenced Jan 2, 2018

Module Unification: Blueprints #7530

Closed

WIP: Use glimmer-wrapper resolver by default for module unification apps #7531

Closed

GavinJoyce restored the gj/module-unification-blueprints branch January 4, 2018 11:39

GavinJoyce deleted the gj/module-unification-blueprints branch February 12, 2018 11:18

		@@ -0,0 +1,5 @@
		<code>src/ui/components/my-component/template.hbs</code>

		@@ -0,0 +1,3 @@
		<code>src/ui/routes/index/template.hbs</code>

		{{my-component}}

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

Conversation

GavinJoyce commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GavinJoyce Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Dec 6, 2017

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinJoyce commented Dec 6, 2017

Uh oh!

GavinJoyce commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GavinJoyce commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Dec 7, 2017

Uh oh!

GavinJoyce commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinJoyce commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinJoyce commented Dec 6, 2017 •

edited

Loading

GavinJoyce Dec 6, 2017 •

edited

Loading

GavinJoyce commented Dec 6, 2017 •

edited

Loading

GavinJoyce commented Dec 6, 2017 •

edited

Loading

GavinJoyce commented Dec 7, 2017 •

edited

Loading

GavinJoyce commented Dec 7, 2017 •

edited

Loading

GavinJoyce commented Dec 7, 2017 •

edited

Loading

ro0gr commented Dec 12, 2017 •

edited

Loading

ro0gr commented Dec 13, 2017 •

edited

Loading

cibernox commented Dec 24, 2017 •

edited

Loading