Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Replace events with generic event 'run' #222

Merged
merged 3 commits into from
Jun 1, 2017

Conversation

helfi92
Copy link
Member

@helfi92 helfi92 commented Jun 1, 2017

No description provided.

@helfi92 helfi92 added this to the v6 milestone Jun 1, 2017
@helfi92 helfi92 self-assigned this Jun 1, 2017
@helfi92 helfi92 requested a review from eliperelman June 1, 2017 16:34
Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

The intent from #166 was to have additional events so we can have a generic event to listen for, but we still need the specific events so things only happen during certain phases. For example, a preset like Jest only wants to run Jest when the test event is ready. If we switch to run only, this wouldn't be possible.

The solution would be to keep the current events, but add 2 additional chains: one for prerun, and one for run.

@codecov
Copy link

codecov bot commented Jun 1, 2017

Codecov Report

Merging #222 into v6-dev will decrease coverage by 0.53%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v6-dev     #222      +/-   ##
==========================================
- Coverage   94.15%   93.61%   -0.54%     
==========================================
  Files          36       36              
  Lines         513      517       +4     
==========================================
+ Hits          483      484       +1     
- Misses         30       33       +3
Impacted Files Coverage Δ
packages/neutrino/src/index.js 65.21% <25%> (-8.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ce3114...9381f6e. Read the comment docs.

@helfi92
Copy link
Member Author

helfi92 commented Jun 1, 2017

Makes sense, sorry I misread.

@helfi92 helfi92 requested a review from eliperelman June 1, 2017 17:16
Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks good so far, just a nit and I think we can merge!

@@ -15,12 +16,18 @@ const run = (command, middleware, options) => {
.map(() => api.config.merge(options.config))
// Trigger all pre-events for the current command
.chain(() => Future.fromPromise2(api.emitForAll, `pre${command}`, api.options.args))
// Trigger generic pre-event
.chain(() => Future.fromPromise2(api.emitForAll, `pre${genericEvent}`, api.options.args))
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with inlining the prerun name here:

.chain(() => Future.fromPromise2(api.emitForAll, `prerun`, api.options.args))

// Execute the command
.chain(() => api.run(command))
// Trigger all post-command events, resolving with the value of the command execution
.chain(value => Future
.fromPromise2(api.emitForAll, command, api.options.args)
.chain(() => Future.of(value)))
// Trigger generic post-event, resolving with the value of the command execution
.chain(value => Future
.fromPromise2(api.emitForAll, genericEvent, api.options.args)
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with inlining the run name here:

.fromPromise2(api.emitForAll, 'run', api.options.args)

@helfi92 helfi92 merged commit 8b3853d into neutrinojs:v6-dev Jun 1, 2017
@helfi92 helfi92 mentioned this pull request Jun 1, 2017
@eliperelman
Copy link
Member

🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants