Skip to content

Module Unification Addons #7490

Merged
rwjblue merged 5 commits intomasterfrom
mu-addons
Mar 5, 2018
Merged

Module Unification Addons #7490
rwjblue merged 5 commits intomasterfrom
mu-addons

Conversation

@rwjblue
Copy link
Copy Markdown
Member

@rwjblue 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 add delayed transpilation #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
@rwjblue rwjblue force-pushed the mu-addons branch 2 times, most recently from 3b029cd to 1aac85f Compare March 1, 2018 19:57
@rwjblue rwjblue force-pushed the mu-addons branch 2 times, most recently from 0bdf691 to 50e835e Compare March 1, 2018 22:45
@rwjblue rwjblue changed the title [WIP] Module Unification Addons Module Unification Addons Mar 1, 2018
Also ensure delayed transpilation works properly with addon's having
/src.
@return {Tree} App file tree
*/
treeForApp(tree) {
if (!experiments.MODULE_UNIFICATION || this.project.isModuleUnification()) {
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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 2 commits March 2, 2018 17:00
 - 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).
improve logic for if addon is module-unification
@@ -0,0 +1,9 @@
module.exports = {
name: 'basic-thing',
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.

@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 */
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not the correct location for styles in MU

@rwjblue rwjblue merged commit d4d586c into master Mar 5, 2018
@bartocc
Copy link
Copy Markdown
Contributor

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 March 5, 2018 14:12
@rwjblue
Copy link
Copy Markdown
Member Author

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants