Skip to content

Module Unification Addon blueprint#7658

Merged
rwjblue merged 1 commit intoember-cli:masterfrom
cibernox:add-module-unification-addon
Mar 7, 2018
Merged

Module Unification Addon blueprint#7658
rwjblue merged 1 commit intoember-cli:masterfrom
cibernox:add-module-unification-addon

Conversation

@cibernox
Copy link
Copy Markdown
Contributor

@cibernox cibernox commented Mar 2, 2018

This adds the ability of generating addons using the module unification layout.

MODULE_UNIFICATION=true ember addon my-addon
MODULE_UNIFICATION=true ember addon my-addon --yarn

@GavinJoyce @mixonic

modulePrefix: name,
namespace,
addonName,
// addonModulePrefix: addonName,
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.

It's not clear to me the difference between addonModulePrefix and addonName and which one should I use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see addonModulePrefix used anywhere in these files.

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.

It is not, I decided to use addon name. But I wanted to understand if there was a good reason for that weird naming

@cibernox
Copy link
Copy Markdown
Contributor Author

cibernox commented Mar 2, 2018

Can someone rebuild this? I think it's a random failure.

@cibernox cibernox force-pushed the add-module-unification-addon branch from 7a276ee to d467039 Compare March 3, 2018 00:47
@GavinJoyce GavinJoyce mentioned this pull request Mar 3, 2018
29 tasks
@cibernox cibernox force-pushed the add-module-unification-addon branch 5 times, most recently from e3fb855 to abb6920 Compare March 6, 2018 17:06
…on layout.

MODULE_UNIFICATION=true ember addon my-addon
MODULE_UNIFICATION=true ember addon my-addon --yarn
@cibernox cibernox force-pushed the add-module-unification-addon branch from abb6920 to e0f4ce5 Compare March 6, 2018 18:19
@@ -0,0 +1,9 @@
The MIT License (MIT)

Copyright (c) 2018
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will fail each year :)

"devDependencies": {
"broccoli-asset-rev": "^2.4.5",
"ember-ajax": "^3.0.0",
"ember-cli": "~3.1.0-beta.1",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will fail each release

Copy link
Copy Markdown
Contributor Author

@cibernox cibernox Mar 7, 2018

Choose a reason for hiding this comment

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

The test is asserting that the same files are being created, not that the contents of those files are the same. Afaik, all other tests are similar.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh, right. there is no verification of result files contents

'use strict';
const addonBlueprint = require('../addon');

module.exports = Object.assign({}, addonBlueprint, {
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.

This is where it all happens. The only difference with the generate of standard addons is the description and the files. I couldn't find a better way of "inheriting" from an existing blueprint.

Copy link
Copy Markdown
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

This looks good to me. @rwjblue ?

@@ -0,0 +1,47 @@
---
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems weird to have both .travis.yml and travis.yml in the diff here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like all of the .foo files have this issue?

Copy link
Copy Markdown
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

I take it all back.

After discussion in chat, please try to have the dummy app be a module unification app (use tests/dummy/src/).

We will have a future hurdle after landing this PR: Addon authors may want to run their acceptance tests with both a MU src/ app and with a classic app/ app. This allows them to test the modern namespaced interface as well as the classic re-exported interface.

Copy link
Copy Markdown
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

DOH! The duplicates I thought I saw were actually test fixtures, sorry about that!

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Mar 7, 2018

Chatted with @mixonic in slack, we agree that this can land and changing the default dummy app setup to be module unification should happen in a follow up PR.

@rwjblue rwjblue merged commit 285f430 into ember-cli:master Mar 7, 2018
@cibernox cibernox deleted the add-module-unification-addon branch March 7, 2018 15:40
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.

4 participants