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 more watcher events #398

Merged
merged 13 commits into from
May 18, 2019
Merged

Add more watcher events #398

merged 13 commits into from
May 18, 2019

Conversation

oligriffiths
Copy link
Contributor

@oligriffiths oligriffiths commented Apr 28, 2019

In order to support the Broccoli watcher in Ember CLI ember-cli/ember-cli#8614, we need more information about the change event and some other events in order to replicate the existing Ember CLI functionality.

I added a test for Watcher as there didn't seem to be one. Plz lmk if I've done anything wrong.

Will add some tests, just wanted to get this open for now.

lib/watcher.js Show resolved Hide resolved
Copy link
Contributor

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

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

LGTM, but add tests ;)

@oligriffiths
Copy link
Contributor Author

@stefanpenner @thoov added tests, this should be good to review now

@oligriffiths
Copy link
Contributor Author

@stefanpenner added a test for Watcher as there doesn't seem to be one. Confused about a couple of things:

  1. What is _lifetimeDeferred about?
  2. If the build promise rejects with a failure, I don't see _error being called
    .catch(err => this._error(err))
    I was debugging this test
    it('calls error if build rejects', function() {
    and asserting that quitStart was called, but it's not. Added some breakpoints and confirmed _error is not called. Is this right?

@oligriffiths
Copy link
Contributor Author

Ugh apvoyer failed, claims tests failed, dunno why...

@thoov
Copy link
Member

thoov commented May 17, 2019

@oligriffiths Looks like the tests are failing for lint reasons. Could you fix that then LGTM!

@thoov thoov self-requested a review May 17, 2019 18:48
@oligriffiths
Copy link
Contributor Author

Will fix. Heading to a flight rn tho

@oligriffiths
Copy link
Contributor Author

@thoov fixed tests. Ready to rock

@thoov thoov merged commit e74e0f0 into master May 18, 2019
@oligriffiths oligriffiths deleted the oli/watcher-events branch May 18, 2019 04:27
@oligriffiths
Copy link
Contributor Author

👏

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.

3 participants