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

Several fixes for the module unification feature flag #6908

Merged
merged 8 commits into from Jul 8, 2017

Conversation

Projects
None yet
6 participants
@mixonic
Contributor

mixonic commented Mar 27, 2017

Several fixes here for the module unification flag, written mostly by @rwjblue with my very watchful supervision.

  • Avoid depending on the app tree being present at all times
  • Create a more realistic src build pipeline
  • Property add src files to the linting system
  • When the src tree is present, defer to src/main.js to declare an application instead of app/app.js.

/cc @rwjblue @Turbo87

@@ -187,7 +187,8 @@ class EmberApp {
let srcPath = this._resolveLocal('src');
let srcTree = existsSync(srcPath) ? new WatchedDir(srcPath) : null;

let appTree = new WatchedDir(this._resolveLocal('app'));
let appPath = this._resolveLocal('app');
let appTree = existsSync(appPath) ? new WatchedDir(appPath) : null;

This comment has been minimized.

@Turbo87

Turbo87 Mar 27, 2017

Member

I'm a little worried about the potential side effects of this change since we don't have that many unit tests for the build pipeline yet and the change is not covered by the feature flag

This comment has been minimized.

@rwjblue

rwjblue Mar 27, 2017

Contributor

Ya, we need to flag this, and add tests to cover these changes generally speaking.

@mixonic mixonic referenced this pull request May 30, 2017

Closed

Module Unification Epic #6332

8 of 13 tasks complete
@homu

This comment has been minimized.

Contributor

homu commented Jun 9, 2017

☔️ The latest upstream changes (presumably #7118) made this pull request unmergeable. Please resolve the merge conflicts.

@mixonic

This comment has been minimized.

Contributor

mixonic commented Jul 2, 2017

This code is some of that last stuff we need to see landed in order to support MU behind feature flags.

@rwjblue rwjblue force-pushed the module-unification-fixes branch from 709a351 to d05346c Jul 6, 2017

rwjblue added some commits Jul 6, 2017

Remove bizarre / brittle test.
Checking that this method was called is super bizarre.

@rwjblue rwjblue force-pushed the module-unification-fixes branch from d05346c to 14174a2 Jul 7, 2017

rwjblue added some commits Jul 7, 2017

@rwjblue

This comment has been minimized.

Contributor

rwjblue commented Jul 7, 2017

Finally got some time to work on this, and I think its ready for another round of reviews.

  • Rebased on top of current master
  • Removed one super bizarre (and misplaced) test
  • Fixed up the previously failing tests
  • Added a few new integration style tests around lib/broccoli/ember-app.js (we can now confirm that EmberApp#appAndDependencies returns the right things in a fast (< 100ms) broccoli-test-helper style test 🎉 )
  • Guarded all new functionality behind the MODULE_UNIFICATION experiment

@mixonic mixonic removed the Needs Tests label Jul 8, 2017

@cibernox

My understanding of this is very small, so I just left a nit pick.

@@ -1127,8 +1183,11 @@ class EmberApp {
let config = this._configTree();
let templates = this._processedTemplatesTree();

let srcTree = experiments.MODULE_UNIFICATION ? this._processedSrcTree() : null;

This comment has been minimized.

@cibernox

cibernox Jul 8, 2017

Contributor

Perhaps move the experiments.MODULE_UNIFICATION check to _processedSrcTree to make it a noop and just call it here blindly?

@rwjblue

rwjblue approved these changes Jul 8, 2017

@mixonic mixonic merged commit ba73fc8 into master Jul 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mixonic mixonic deleted the module-unification-fixes branch Jul 8, 2017

@mixonic mixonic restored the module-unification-fixes branch Jul 8, 2017

@locks locks deleted the module-unification-fixes branch Jul 9, 2017

@twokul

This comment has been minimized.

Contributor

twokul commented Jul 18, 2017

@mixonic @rwjblue I must be reading the code wrong? experiments.MODULE_UNIFICATION always returns something (__module-unification__ [id=EMBER_CLI91203924408] for example). That means check like this won't work:

if (!experiments.MODULE_UNIFICATION) {
  return null;
}

and you always have to pair it with another guard !this.trees.src

@Turbo87

This comment has been minimized.

Member

Turbo87 commented Jul 18, 2017

@twokul experiments are disabled on the beta and release branches

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