Skip to content
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

Add Broccoli 2.0 support #7798

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Conversation

oligriffiths
Copy link
Contributor

@oligriffiths oligriffiths commented Apr 30, 2018

What

Once broccolijs/broccoli#363 merges, this PR adds support for broccoli 2.0 in ember-cli via BROCCOLI_2 environment flag. If there is a better way to do this, please LMK and I will change.

The requisite change is to require the broccoli package, and to reformat the response from the broccoli build() method, into:

{
  directory: node.outputPath,
  graph: node,
}

Test

In my testing, the output of both the 0-18x series and master is the same, I verified this with:

ember build dist1
EMBER_CLI_BROCCOLI_2=1 ember build dist2
diff -rq dist1 dist2

It should output nothing.

To test using the system temp directory, do:

EMBER_CLI_BROCCOLI_2=1 EMBER_CLI_SYSTEM_TEMP=1 ember build

Updated tests to validate the build results are the same, and support for system tmp dir.

@oligriffiths oligriffiths changed the title Add BROCCOLI_MASTER env var to support broccoli 1.0 Add Broccoli 1.0 support Apr 30, 2018
@Turbo87
Copy link
Member

Turbo87 commented Apr 30, 2018

nice work! 👏

via BROCCOLI_MASTER environment flag. If there is a better way to do this, please LMK and I will change

we usually use https://github.com/ember-cli/ember-cli/blob/master/lib/experiments/index.js for these things, though that does not use env vars (yet). I would recommend migrating the feature flag to that file but instead of using the symbol() code there keep using the env var that you already have.

@oligriffiths
Copy link
Contributor Author

@Turbo87 Done. What's the best way to CI test this? Is it possible to re-run all tests with and without the env var set?

@Turbo87
Copy link
Member

Turbo87 commented May 1, 2018

What's the best way to CI test this? Is it possible to re-run all tests with and without the env var set?

What we currently do is: on master all experiments are enabled, on beta and release all experiments are supposed to be disabled. That led to beta currently being broken since the tests passed with the Module Unification experiment being enabled, but when I disabled the experiment for the beta release the test suite started breaking.

What I'm meaning to say: what we currently do is not great and I think running the test suite with experiments turned off by default would be better and then adding another CI job per experiment.

@ember-cli/core thoughts?

@mansona
Copy link
Member

mansona commented May 1, 2018

@Turbo87 I don't know how crazy you want to get with the CI setup but I think that we should be considering running a full on/off matrix of experiments on each CI build, that way we would catch any potential interactions between experiments.

I don't think it needs to go so far as to test each node version with the full matrix of experiments but maybe we could pick one node version and then run the experiment matrix off it as a smoke test?

The current issue with the Beta branch was introduced on 1st March and would have shown on any test that disabled the MU experiment.

I'm happy to help set this up 😄

@Turbo87
Copy link
Member

Turbo87 commented May 1, 2018

My approach would be:

  • one job per node supported version running without experiments
  • one job per experiment running the fastest node version (10)
  • one job running with all experiments enabled on the fastest node version

Certainly can't catch all issues, but seems like a reasonable compromise. PR welcome, but we should discuss the details in the CLI call on Thursday.

@rwjblue
Copy link
Member

rwjblue commented May 1, 2018

@Turbo87 - I completely agree with the suggested path forward in your last comment (#7798 (comment)).

@oligriffiths
Copy link
Contributor Author

This approach seems to make sense and provides a decent balance between every permutation of experiments thus taking longer to the suite to pass and coverage of conditions.

In my case, there’s a bug somewhere as the test suite doesn’t pass with the experiment enabled.

@rwjblue
Copy link
Member

rwjblue commented May 1, 2018

I submitted #7803 for refactoring the CI runs...

rwjblue
rwjblue previously requested changes May 2, 2018
Copy link
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.

Very excited for this progress!

@@ -6,6 +6,7 @@ const symbol = require('../utilities/symbol');
let experiments = {
MODULE_UNIFICATION: symbol('module-unification'),
DELAYED_TRANSPILATION: symbol('delayed-transpilation'),
BROCCOLI_MASTER: process.env.BROCCOLI_MASTER === '1', // Note: EXPERIMENTAL, DO NOT USE!
Copy link
Member

Choose a reason for hiding this comment

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

Hehe, these are all experimental 😉

Copy link
Member

Choose a reason for hiding this comment

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

Lets make the flag: process.env.EMBER_CLI_BROCCOLI_1 (I don't like using _MASTER conceptually here)...

@@ -44,7 +44,12 @@ class Builder extends CoreObject {
this.environment = this.environment || 'development';
process.env.EMBER_ENV = process.env.EMBER_ENV || this.environment;

const broccoli = require('broccoli-builder');
let broccoli;
if (this.broccoliMaster) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather do the experiments check here, instead of in the build task

if (experiments.BROCCOLI_MASTER) {

} else {

}

@@ -152,6 +157,17 @@ class Builder extends CoreObject {

return this.processAddonBuildSteps('preBuild')
.then(() => this.builder.build(willReadStringDir))
.then(node => {
// broccoli-builder reformats the response into {directory, graph}
if (this.broccoliMaster) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets do the experiments guard here directly also

@@ -17,6 +18,7 @@ class BuildTask extends Task {
outputPath: options.outputPath,
environment: options.environment,
project: this.project,
broccoliMaster: experiments.BROCCOLI_MASTER,
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to revert changes to this file once the builder itself does its own experiments check...

@@ -26,6 +27,7 @@ class ServeTask extends Task {
outputPath: options.outputPath,
project: this.project,
environment: options.environment,
broccoliMaster: experiments.BROCCOLI_MASTER,
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to revert changes to this file once the builder itself does its own experiments check...

});
} else {
this._cleanupPromise = this.builder.cleanup();
}
Copy link
Member

Choose a reason for hiding this comment

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

you can simplify this using:

this._cleanupPromise = Promise.resolve().then(() => this.builder.cleanup());

which should work for both Broccoli 0.x and 1.x

@@ -148,13 +149,15 @@ describe('models/builder.js', function() {
});
});

it('calls instrumentation.stop(build, result, resultAnnotation)', function() {
let mockAnnotation = 'MockAnnotation';
if (!experiments.BROCCOLI_MASTER) {
Copy link
Member

Choose a reason for hiding this comment

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

this should be BROCCOLI_1, right?

it('allows addons to add promises preBuild', function() {
let preBuild = td.replace(addon, 'preBuild', td.function());
td.when(preBuild(), { ignoreExtraArgs: true, times: 1 }).thenReturn(Promise.resolve());
if (!experiments.BROCCOLI_MASTER) {
Copy link
Member

Choose a reason for hiding this comment

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

see above

@rwjblue
Copy link
Member

rwjblue commented Jun 11, 2018

FYI - I just released broccoli@2.0.0-beta.2 (updates deps, drops RSVP, drops Node 4).

@oligriffiths
Copy link
Contributor Author

👌

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2018

@oligriffiths - I just rebased, added the bump commit to broccoli@2.0.0-beta.2, and force pushed to kick off a new CI run. I think this is very close to being landable...

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2018

Restarted some of the CI builds (they were failing on the yarn installation)...

@oligriffiths
Copy link
Contributor Author

@rwjblue thanks, I saw that, good to know you can manually restart them, was gonna do a noop commit

bin/ember Outdated
}).catch((err) => {
// should not normally reach here
// this handles the case where console-ui is broken
console.error(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general we would prefer to not mutate this file. But this may be an exception, @rwjblue c/d?

Copy link
Member

Choose a reason for hiding this comment

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

Chatted with @krisselden in slack about this, I think we can probably move this to lib/cli.js and still handle the broken scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@@ -26,7 +27,11 @@ class Builder extends CoreObject {
constructor(options) {
super(options);

this.broccoli2 = Boolean(options.broccoli2 === undefined ? experiments.BROCCOLI_2 : options.broccoli2);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to remove this.broccoli2 completely, and replace it with direct experiments.BROCCOLI_2 checks...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn’t see how to mock that property in tests. I want to compare with the property on and off in the same test.

Copy link
Member

Choose a reason for hiding this comment

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

Why? I think having tests around the expected behaviors should be enough. In general, from the outside, I don't think which builder we use should actually matter much, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we want to ensure that the files are exactly the same between both versions.
I guess if we have a fixture directory and compare against that, that'd also work.

Copy link
Member

Choose a reason for hiding this comment

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

Totally agreed. I think the goal here should be that the migration is seemless and the output is identical. And we should be able to do that via fixtures (either inside the file system, or just via fixturify in the test itself)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Are we doing this anywhere already?

@@ -26,7 +27,11 @@ class Builder extends CoreObject {
constructor(options) {
super(options);

this.broccoli2 = Boolean(options.broccoli2 === undefined ? experiments.BROCCOLI_2 : options.broccoli2);
this.systemTemp = Boolean(options.systemTemp === undefined ? experiments.SYSTEM_TEMP : options.systemTemp);
Copy link
Member

Choose a reason for hiding this comment

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

Same RE: experiment checks. In general it should not be possible to force these experiments to true without process.env.EMBER_CLI_* is set...

* @param node The node returned from Broccoli builder
*/
compatNode(node) {
if (this.broccoli2) {
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping that we could structure the code such that the broccoli@2 case is the "normal" case, and we do any shimming/polyfilling for the older broccoli-builder APIs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should do that once we move out of experimental mode

package.json Outdated
@@ -130,6 +131,7 @@
"chai-as-promised": "^7.0.0",
"chai-files": "^1.2.0",
"co": "^4.6.0",
"dir-compare": "^1.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

What are we using this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comparing the directory structure and files for a Broccoli-builder vs broccoli 2.0 build to ensure they’re identical.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think in general I'd prefer to use something like:

expect(fixturify.readSync(oneDir)).to.deep.equal(fixturify.readSync(twoDir))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the more you know. Will test that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will that compare file contents/hash?

Copy link
Member

Choose a reason for hiding this comment

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

fixturify.readSync loads an entire directory (recursively) into a POJO (where the values are the string contents of the individual files).

Snippet from the readme:

var fixturify = require('fixturify')

var obj = {
  'foo.txt': 'foo.txt contents',
  'subdir': {
    'bar.txt': 'bar.txt contents'
  }
}

fixturify.writeSync('testdir', obj) // write it to disk

fixturify.readSync('testdir') // => deep-equals obj

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue are there already tests that already assert the output of the build is correct?

@@ -66,6 +66,9 @@ class Builder extends CoreObject {
};
} else {
broccoli = require('broccoli-builder');
if (this.systemTemp) {
console.warn('EMBER_CLI_SYSTEM_TEMP only works in combination with EMBER_CLI_BROCCOLI_2');
Copy link
Contributor

Choose a reason for hiding this comment

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

we should simply error here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need to isolate tests in that case, because we can't do only combo environment variables (e.g. BROCCOLI_2 and SYSTEM_TEMP). Right now, the SYSTEM_TEMP flag will cause this error when being run without setting BROCCOLI_2.
Will do a little refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (experiments.BROCCOLI_2) {
expect(result.graph.__heimdall__).to.not.be.undefined;
} else {
expect(result.graph.constructor.name).to.equal('Node');
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand the question...

@rwjblue
Copy link
Member

rwjblue commented Jun 27, 2018

Updated to ensure SYSTEM_TEMP experiment only runs with BROCCOLI_2.

@rwjblue
Copy link
Member

rwjblue commented Jun 27, 2018

Just rebased against master to get some of the CI fixes that have landed...

@oligriffiths oligriffiths force-pushed the broccoli-master branch 3 times, most recently from 7cd2082 to 8607dda Compare June 29, 2018 16:04
@oligriffiths
Copy link
Contributor Author

@rwjblue tests all passing 🎉

@oligriffiths
Copy link
Contributor Author

@krisselden I had to remove your changes to get tests passing. Things have changed a bit since you implemented them, and im not 100% sure what they were changing. So if you wouldn't mind testing this branch now, and see if you get the same errors, I'd appreciate it.

@oligriffiths oligriffiths dismissed rwjblue’s stale review July 2, 2018 17:57

Ok, lots of changes since this review, gonna dismiss and re-tag

@oligriffiths oligriffiths requested a review from rwjblue July 2, 2018 17:57
@rwjblue
Copy link
Member

rwjblue commented Jul 12, 2018

Rebased and updated broccoli to 2.0.0-beta.3...

@rwjblue rwjblue merged commit 819f15e into ember-cli:master Jul 12, 2018
@oligriffiths
Copy link
Contributor Author

Whoop 🎉

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.

None yet

6 participants