Skip to content

Add project.isModuleUnification()#7541

Merged
twokul merged 1 commit intoember-cli:masterfrom
GavinJoyce:gj/mu
Jan 4, 2018
Merged

Add project.isModuleUnification()#7541
twokul merged 1 commit intoember-cli:masterfrom
GavinJoyce:gj/mu

Conversation

@GavinJoyce
Copy link
Copy Markdown
Contributor

@GavinJoyce GavinJoyce commented Jan 4, 2018

UPDATE: if you're adding this to a beta, please also include #7586 which adds a feature flag check.

part of emberjs/ember.js#16043
(which is part of #7530)

This PR adds initial support for ember g * commands within a module unification project. The existence of a src directory in the ember project determines if that project is a module unification project. After this is merged, the next step will be to update the Ember blueprints to support MU output by checking project.isModuleUnification()

/cc @rwjblue

@GavinJoyce GavinJoyce force-pushed the gj/mu branch 3 times, most recently from 9407089 to 0d68dbc Compare January 4, 2018 14:22
@GavinJoyce GavinJoyce force-pushed the gj/mu branch 2 times, most recently from 16e4889 to 63642e4 Compare January 4, 2018 15:31
@GavinJoyce GavinJoyce changed the title [WIP] Support ember generate commands from a module unification project Support ember generate commands from a module unification project Jan 4, 2018

it('prefixes `component`', function() {
let name = task.prefixBlueprintName('component');
expect(name).to.equal('module-unification-component');
Copy link
Copy Markdown

@alexander-alvarez alexander-alvarez Jan 4, 2018

Choose a reason for hiding this comment

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

Should this not still be component? Just curious what we gain from the separation.
We could probably hook into isModuleUnification that you added and override __path__ and __templatepath__ in the ember.js component blueprint

Copy link
Copy Markdown
Contributor Author

@GavinJoyce GavinJoyce Jan 4, 2018

Choose a reason for hiding this comment

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

It could be a valid approach, yes. I think it's cleaner to keep these blueprints separate though, especially given that there isn't much reuse between the contents of the files. The plan would be to rename module-unification-component to component once MU is the only type of project

Copy link
Copy Markdown
Contributor Author

@GavinJoyce GavinJoyce Jan 4, 2018

Choose a reason for hiding this comment

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

A downside of my approach is that there is a coupling between ember-cli and ember (you would need to update both to use the new MU blueprints and again when we rename module-unification-component to component in future). Your suggested approach doesn't have this issue when doing the rename, just for the initial case.

I'll try your approach as see how it looks, thanks

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would need to update both to use the new MU blueprints and again when we rename ..

I wouldn't say that's a downside to your approach, as we could pollyfill the MU blueprints, and issue a deprecation warning when the hosts' app version doesn't need the polyfill anymore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

true

Copy link
Copy Markdown

@alexander-alvarez alexander-alvarez Jan 4, 2018

Choose a reason for hiding this comment

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

I think the big difference is that we can experiment and build blueprints with isModuleUnification in ember (or even addons) without having to enable each different type of MU-blueprint here https://github.com/ember-cli/ember-cli/pull/7541/files#diff-c3643df1b2f6f7f28a101393e4fe0c12R99

Already we can support many permutations of the same file type like we do here https://github.com/emberjs/ember.js/tree/master/blueprints/controller-test (e.g we could have a module-unification-files prefix in component, etc)

Copy link
Copy Markdown
Contributor Author

@GavinJoyce GavinJoyce Jan 4, 2018

Choose a reason for hiding this comment

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

👍

I don't believe that we have access to the project in the blueprints, I'll add a options.moduleUnification property and refactor the component blueprint

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

great, thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for your feedback, updated

@GavinJoyce GavinJoyce changed the title Support ember generate commands from a module unification project Add project.isModuleUnification() Jan 4, 2018
Copy link
Copy Markdown

@alexander-alvarez alexander-alvarez left a comment

Choose a reason for hiding this comment

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

👍

return !!this.pkg.keywords && this.pkg.keywords.indexOf('ember-addon') > -1;
}

isModuleUnification() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

documentation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks, updated

return !!this.pkg.keywords && this.pkg.keywords.indexOf('ember-addon') > -1;
}

isModuleUnification() {
Copy link
Copy Markdown
Contributor

@ro0gr ro0gr Jan 4, 2018

Choose a reason for hiding this comment

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

is it supposed to be a public API? Maybe it has to be marked as private or protected in order to avoid following deprecation when isModuleUnification becomes default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I think it should be private as it's just a temporary internal API to allow us to branch the blueprints.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants