Skip to content
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

Module-unification component blueprint #16043

Merged
merged 1 commit into from Feb 19, 2018
Merged

Module-unification component blueprint #16043

merged 1 commit into from Feb 19, 2018

Conversation

GavinJoyce
Copy link
Member

@GavinJoyce GavinJoyce commented Jan 2, 2018

Part of ember-cli/ember-cli#7530

This PR adds initial support for ember g component foo-bar from a module unification app.

TODO:

QUESTIONS:

USEFUL LINKS:

@rwjblue
Copy link
Member

rwjblue commented Jan 2, 2018

CI failure is due to eslint not being able to parse our templating, I believe we need to disable eslint from running on blueprints/**/files/...

package.json Outdated
@@ -94,7 +94,7 @@
"chalk": "^2.3.0",
"common-tags": "^1.4.0",
"dag-map": "^2.0.2",
"ember-cli": "2.10.0",
"ember-cli": "github:ember-cli/ember-cli",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

project.isModuleUnification() was just recently merged to master

return {
__root__(options) {
if (isModuleUnification) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check will break --dummy and --in-repo-addon cli flags. So if we have a module unification layout then

  ember g component some-component --dummy 

will generate component in the src/ instead of dummy/ dir.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I've just realized that dummy apps has to support module unification layout as well 🤔

Copy link
Member Author

@GavinJoyce GavinJoyce Jan 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes. I have an Add support for addons todo item above and I've asked a question on #st-module-unification looking for an example MU addon so that I can add support for it

@GavinJoyce GavinJoyce changed the title [WIP] added module-unification-component blueprint [WIP] module-unification component blueprint Feb 15, 2018
@GavinJoyce
Copy link
Member Author

This ready for review. There is one small remaining item to point at the next beta release of ember-cli whenever it's released

package.json Outdated
@@ -100,7 +100,7 @@
"chalk": "^2.3.0",
"common-tags": "^1.7.2",
"dag-map": "^2.0.2",
"ember-cli": "2.18.0",
"ember-cli": "github:ember-cli/ember-cli",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to keep this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, ideally we'll update this to the next ember cli beta (it's the sole outstanding todo item on this PR)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I read what was written but didn't understand properly.
the work looks good 😃

@GavinJoyce GavinJoyce changed the title [WIP] module-unification component blueprint Module-unification component blueprint Feb 16, 2018
package.json Outdated
@@ -100,7 +100,7 @@
"chalk": "^2.3.0",
"common-tags": "^1.7.2",
"dag-map": "^2.0.2",
"ember-cli": "2.18.0",
"ember-cli": "github:ember-cli/ember-cli#6a85734c52d129fc0824ec3fbf21bd01f9c9fa38",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't point to 3.1.0-beta.1 as intended as experiments are removed from betas and releases. Pointing to ember-cli/ember-cli@6a85734 instead

@GavinJoyce
Copy link
Member Author

GavinJoyce commented Feb 16, 2018

Hm, there seems to be some failing tests around differences with destroyApp in unrelated tests. I'll dig into it later this evening

@GavinJoyce GavinJoyce changed the title Module-unification component blueprint [wip] Module-unification component blueprint Feb 16, 2018
@GavinJoyce
Copy link
Member Author

There are a couple of things preventing me from currently upgrading to ember-cli 3.1.0-beta.1:

Some options:

/cc @mixonic as you were keen for me to add the experiment check in project.isModuleUnification.

@GavinJoyce GavinJoyce changed the title [wip] Module-unification component blueprint Module-unification component blueprint Feb 19, 2018
@mixonic
Copy link
Sponsor Member

mixonic commented Feb 19, 2018

We discussed this at the MU strike team meeting this week. The SHA pinned in this PR is before several changes where made to Ember-CLI that seem to break the Ember blueprints. I'm worried that breakage could be impacting real apps (on Canary if not anything stable) when using generators.

However that breakage is not related to what @GavinJoyce has been digging into here.

This looks great and is wholly behind a feature flag. Let's continue to iterate in master. Thanks @GavinJoyce!

@mixonic mixonic merged commit b05e16d into emberjs:master Feb 19, 2018
@GavinJoyce GavinJoyce deleted the gj/mu-component-blueprint branch February 19, 2018 20:30
@@ -6,6 +6,7 @@ const isPackageMissing = require('ember-cli-is-package-missing');
const getPathOption = require('ember-cli-get-component-path-option');

const useTestFrameworkDetector = require('../test-framework-detector');
const { isModuleUnificationProject } = require('../module-unification');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break on node@4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ro0gr, I'll update now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants