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 Addons #7490

Merged
merged 5 commits into from Mar 5, 2018

Conversation

Projects
None yet
5 participants
@rwjblue
Copy link
Contributor

rwjblue commented Dec 6, 2017

Todo:

  • Dry up lib/broccoli/ember-addon.js (copied from lib/broccoli/ember-app.js)
  • Process styles (in some way) from src/ in addon (models/addon.js)
  • fix bad error message for Missing template processor when missing ember-cli-htmlbars in dependecies i.e. it exists in dev-dependencies
  • throw an error (or do something) when an addon can't decide if it's mu or classic (i.e. has src/ and addon/ or app/ folders) @rwjblue and @mixonic think having all three dirs is a valid case. We're going to leave the current ambiguous behavior.
  • ensure src processing is lazy (per #7501)

Tests:

  • addons with src/ folder in classic apps

Future TODOS:

iezer added a commit to iezer/golden-addon that referenced this pull request Dec 22, 2017

@mansona mansona referenced this pull request Jan 22, 2018

Open

Module Unification - Broccoli Todo List #7568

0 of 2 tasks complete

@rwjblue rwjblue force-pushed the mu-addons branch 2 times, most recently from 3b029cd to 1aac85f Mar 1, 2018

@rwjblue rwjblue force-pushed the mu-addons branch 2 times, most recently from 0bdf691 to 50e835e Mar 1, 2018

@rwjblue rwjblue changed the title [WIP] Module Unification Addons Module Unification Addons Mar 1, 2018

Reduce duplication in ember-addon / ember-app.
Also ensure delayed transpilation works properly with addon's having
/src.

@rwjblue rwjblue force-pushed the mu-addons branch from 50e835e to 0ab0967 Mar 1, 2018

@@ -699,6 +702,24 @@ let addonProto = {
@return {Tree} App file tree
*/
treeForApp(tree) {
if (!experiments.MODULE_UNIFICATION || this.project.isModuleUnification()) {

This comment has been minimized.

@iezer

iezer Mar 2, 2018

Contributor

pretty sure this should be || !this.project.isModuleUnification()) with a not (!)

This comment has been minimized.

@rwjblue

rwjblue Mar 2, 2018

Contributor

discussed in slack, I think we actually need the following guard:

if (!experiments.MODULE_UNIFICATION) {
  return tree;
} else if (project.isModuleUnification() && this.isModuleUnification()) {
  return null;
}

And add a new isModuleUnification method to the addon base class...

iezer and others added some commits Mar 2, 2018

improve logic for if addon is module-unification
 - required so that a module-unification app can handle a mix of addons
   which are module-unification (in which case they have a src folder
   but no app folder), or classic (in which case they have an app folder).
Merge pull request #7660 from iezer/mu-addons
improve logic for if addon is module-unification
@@ -0,0 +1,9 @@
module.exports = {
name: 'basic-thing',

This comment has been minimized.

@iezer

iezer Mar 2, 2018

Contributor

@mixonic I wonder if these tests are failing because the name doesn't match the folder name. In the other examples they do.
PS how do I run just these tests locally, and even better is it possible to run them in the browser?

@@ -0,0 +1 @@
/* addon/styles/app.css is present */

This comment has been minimized.

@rwjblue

rwjblue Mar 4, 2018

Contributor

This is not the correct location for styles in MU

@rwjblue rwjblue merged commit d4d586c into master Mar 5, 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 decreased (-0.3%) to 92.242%
Details
@bartocc

This comment has been minimized.

Copy link

bartocc commented Mar 5, 2018

@rwjblue is this PR supposed to let addon authors write addons with the MU project structure ?

@rwjblue rwjblue deleted the mu-addons branch Mar 5, 2018

@rwjblue

This comment has been minimized.

Copy link
Contributor

rwjblue commented Mar 5, 2018

@bartocc - Yes, it is the first baby step towards that goal.

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