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 Addon blueprint #7658

Merged
merged 1 commit into from Mar 7, 2018

Conversation

Projects
None yet
4 participants
@cibernox
Copy link
Contributor

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,

This comment has been minimized.

@cibernox

cibernox Mar 2, 2018

Contributor

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

This comment has been minimized.

@mixonic

mixonic Mar 2, 2018

Contributor

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

This comment has been minimized.

@cibernox

cibernox Mar 2, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

cibernox commented Mar 2, 2018

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

@cibernox cibernox force-pushed the cibernox:add-module-unification-addon branch from 7a276ee to d467039 Mar 3, 2018

@GavinJoyce GavinJoyce referenced this pull request Mar 3, 2018

Open

[QUEST] Module Unification: Blueprints #7530

18 of 29 tasks complete

@cibernox cibernox force-pushed the cibernox:add-module-unification-addon branch 5 times, most recently from e3fb855 to abb6920 Mar 5, 2018

This adds the ability of generating addons using the module unificati…
…on layout.

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

@cibernox cibernox force-pushed the cibernox:add-module-unification-addon branch from abb6920 to e0f4ce5 Mar 6, 2018

@@ -0,0 +1,9 @@
The MIT License (MIT)

Copyright (c) 2018

This comment has been minimized.

@ro0gr

ro0gr Mar 7, 2018

Contributor

it will fail each year :)

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

This comment has been minimized.

@ro0gr

ro0gr Mar 7, 2018

Contributor

it will fail each release

This comment has been minimized.

@cibernox

cibernox Mar 7, 2018

Contributor

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.

This comment has been minimized.

@ro0gr

ro0gr Mar 7, 2018

Contributor

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

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

module.exports = Object.assign({}, addonBlueprint, {

This comment has been minimized.

@cibernox

cibernox Mar 7, 2018

Contributor

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.

@mixonic

mixonic approved these changes Mar 7, 2018

Copy link
Contributor

mixonic left a comment

This looks good to me. @rwjblue ?

@@ -0,0 +1,47 @@
---

This comment has been minimized.

@rwjblue

rwjblue Mar 7, 2018

Contributor

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

This comment has been minimized.

@rwjblue

rwjblue Mar 7, 2018

Contributor

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

@mixonic
Copy link
Contributor

mixonic left a comment

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.

@rwjblue

rwjblue approved these changes Mar 7, 2018

Copy link
Contributor

rwjblue left a comment

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

@rwjblue

This comment has been minimized.

Copy link
Contributor

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

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.54%
Details

@cibernox cibernox deleted the cibernox:add-module-unification-addon branch Mar 7, 2018

@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