8000 Catch InvalidNodeError for Broccoli 2.0 and fallback to broccoli-builder by oligriffiths · Pull Request #8083 · ember-cli/ember-cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Catch InvalidNodeError for Broccoli 2.0 and fallback to broccoli-builder #8083

10000 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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2018
Merged

Catch InvalidNodeError for Broccoli 2.0 and fallback to broccoli-builder #8083

merged 1 commit into from
Oct 10, 2018

Conversation

oligriffiths
Copy link
Contributor
@oligriffiths oligriffiths commented Oct 1, 2018 •
edited
Loading

As the title says. This should provide a decent level of BC for users with old plugins.

This can be tested with a new ember project, and yarn linking this branch of ember-cli into the new project.

Then ember install ember-browserify which uses the old read/rebuild API

Then ember build results in:

image

Now this might be too much output, perhaps we only need the ERROR, given that broccoli-builder also outputs the offending plugin.

Thoughts...

rwjblue
rwjblue previously requested changes Oct 1, 2018
View reviewed changes
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.

Great idea! Can you retarget and rebase this against beta branch?

lib/models/builder.js Outdated
}

// Fallback to broccoli-builder
console.log('');
Copy link
Member

Choose a reason for hiding this comment

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

We should use this.ui instead of console (comes from https://github.com/ember-cli/console-ui)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point me to an implementation? this.ui is undefined

Copy link
Member

Choose a reason for hiding this comment

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

Each of the tasks that instantiates this builder model (build, build-watch, serve) pass ui, it should_ be present.

ember-cli/lib/tasks/build.js

Lines 15 to 18 in 2805962

let builder = new Builder({
ui,
outputPath: options.outputPath,
environment: options.environment,

I’m not sure how it would end up undefined. Can you confirm that the environment that you were testing it in was setup correctly? I could imagine for example that some tests may not instantiate with the correct arguments...

lib/models/builder.js Outdated
console.warn(chalk.yellow('---------------'));
console.warn(chalk.yellow(e.message));
console.warn(chalk.yellow('---------------'));
this.broccoli2 = false;
Copy link
Member

Choose a reason for hiding this comment

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

I just removed these properties in beta branch, I'll merge upwards so that shows up on master in a few minutes

@stefanpenner stefanpenner mentioned this pull request Oct 3, 2018
Closed
@oligriffiths oligriffiths force-pushed the broccoli-2-node-error branch from 90d4b97 to 4e26d3b Compare October 4, 2018 20:08
@oligriffiths
Copy link
Contributor Author
oligriffiths commented Oct 4, 2018 •
edited
Loading

I wonder if we should catch all exceptions as a result of setting up the broccoli 2 builder, for full fallback support, not just the read/rebuild api. @stefanpenner @rwjblue thoughts?

@oligriffiths
Copy link
Contributor Author

As Stefan said, let's start with catching only the needed and dial it up from there.

@oligriffiths oligriffiths dismissed rwjblue’s stale review October 5, 2018 22:46

Comments addressed, we do need some state to tell the rest of the builder whether we're 10000 in fallback mode, so added that, please check it out.

@oligriffiths oligriffiths requested a review from rwjblue October 5, 2018 22:46
rwjblue
rwjblue requested changes Oct 6, 2018
View reviewed changes
Copy link
Member
@rwjblue rwjblue left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some inline comments, but also need to fix:

  • Update PR to point to beta branch
  • Rebase against beta
  • Add real world test using our builder with broccoli-test-helper along with an old style plugin (we should still build)

lib/models/builder.js Outdated
if (isExperimentEnabled('BROCCOLI_2')) {
broccoli = require('broccoli');
let tmpDir;

// If not using system temp dir, compatability mode with broccoli-builder, tmp in root
if (!isExperimentEnabled('SYSTEM_TEMP')) {
if (!systemTemp) {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not cache experiment values. Each branch point should call isExperimentEnabled directly.

lib/models/builder.js Outdated
this.tree = buildFile({ project: this.project });
} else {
throw new SilentError('No ember-cli-build.js found.');
if (systemTemp) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don’t cache the experiment values, always check isExperimentEnabled

lib/models/builder.js Outdated
}

// Fallback to broccoli-builder
console.log('');
Copy link
Member

Choose a reason for hiding this comment

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

Each of the tasks that instantiates this builder model (build, build-watch, serve) pass ui, it should_ be present.

ember-cli/lib/tasks/build.js

Lines 15 to 18 in 2805962

let builder = new Builder({
ui,
outputPath: options.outputPath,
environment: options.environment,

I’m not sure how it would end up undefined. Can you confirm that the environment that you were testing it in was setup correctly? I could imagine for example that some tests may not instantiate with the correct arguments...

tests/unit/models/builder-test.js Outdated
if (isExperimentEnabled('BROCCOLI_2')) {
describe('fallback from broccoli 2 to broccoli-builder', function() {
it('falls back to broccoli-builder if an InvalidNode error is thrown for read/rebuild api', function() {
let spy = sinon.spy(console, 'warn');
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 removed once the tests properly instantiate the builder with our MockUI object. You can reference other tests to see how to make assertions on the output when using it.

@oligriffiths oligriffiths changed the base branch from master to beta October 7, 2018 09:48
@oligriffiths oligriffiths force-pushed the broccoli-2-node-error branch 2 times, most recently from ed66e87 to 656291d Compare October 7, 2018 09:58
@rwjblue
Copy link
Member
rwjblue commented Oct 7, 2018

I pushed a commit that replaces console usage with console-ui (and fixed up the tests that were not properly passing in the ui object).

Only thing left here is a "real world test".

@oligriffiths
Copy link
Contributor Author

@rwjblue cool, thanks for that update.

RE: Add real world test using our builder with broccoli-test-helper I'm not quite sure what you're asking for, mind elaborating?

@rwjblue
Copy link
Member
rwjblue commented Oct 8, 2018

The only tests that exist for the fallback behavior are very mock heavy. We should be able to author a test that uses the builder model with a pipeline that actually includes a node with the .read API, and confirm that the final build output is actually correct (along with the console output).

@oligriffiths
Copy link
Contributor Author

@rwjblue something like this? https://github.com/ember-cli/ember-cli/pull/8083/files#diff-32c532ce82c7b2f25aa8a1be7fa97672R259https://github.com/ember-cli/ember-cli/pull/8083/files#diff-32c532ce82c7b2f25aa8a1be7fa97672R259 https://github.com/ember-cli/ember-cli/pull/8083/files#diff-ffcb7262da504ca7263dec73783976b0

@rwjblue
Copy link
Member
6D47 rwjblue commented Oct 8, 2018

Ya that test definitely ticks the box, but I was generally looking for a test that uses broccoli-test-helper directly. I migrated the test you added into that format and updated.

This is ready to go once CI is happy...

@oligriffiths
Copy link
Contributor Author

@rwjblue nice, makes sense. Not sure what’s up with CI, 51 failures, not 100% sure what the issue is

@rwjblue
Copy link
Member
rwjblue commented Oct 8, 2018

Ya, seems weird, I restarted CI...

@rwjblue
Copy link
Member
rwjblue commented Oct 8, 2018

OK, I think I figured it out. lib/utilities/find-build-file.js does a process.chdir that I wasn't resetting after adding the new test (which left process.cwd() in $TMPDIR).

@rwjblue
Copy link
Member
rwjblue commented Oct 9, 2018

Remaining CI failure is being tracked over in #8098. It does not seem related to this PR (the same test is failing in beta and master branches).

@oligriffiths
Copy link
Contributor Author

Ok cool. Do we need to wait until CI passes to merge then?

@rwjblue
Copy link
Member
rwjblue commented Oct 9, 2018

Hopefully getting to passing today. Fell asleep last night doing a bisect (release is good, beta is bad), only 3 more steps to go though.

@oligriffiths
Copy link
Contributor Author
oligriffiths commented Oct 9, 2018 •
edited
Loading

What a way to fall asleep 😂
Most people just count sheep

@oligriffiths oligriffiths force-pushed the broccoli-2-node-error branch from f7eaa88 to 0922e31 Compare October 10, 2018 11:33
@oligriffiths oligriffiths force-pushed the broccoli-2-node-error branch from 0922e31 to b375cc8 Compare October 10, 2018 11:37
@rwjblue rwjblue merged commit 0d173ae into ember-cli:beta Oct 10, 2018
@oligriffiths oligriffiths deleted the broccoli-2-node-error branch October 10, 2018 21:53
@bertdeblock bertdeblock mentioned this pull request Dec 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanpenner stefanpenner stefanpenner approved these changes

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oligriffiths @rwjblue @stefanpenner
0