Core: Add support for multiple hooks #1188
Conversation
There was a problem hiding this comment.
I'm super excited this, thank you for tackling it @trentmwillis! This should absolutely unblock the much nicer (IMO) API leveraging nested modules that I proposed in emberjs/ember-qunit#258.
I left a few small questions/comments (feel free to disregard as appropriate).
src/core.js
Outdated
|
|
||
| function setHookFromEnvironment( hooks, environment, name ) { | ||
| const potentialHook = environment[ name ]; | ||
| hooks[ name ] = typeof potentialHook === "function" ? [ potentialHook ] : []; |
There was a problem hiding this comment.
I'm unsure if it matters, but this creates 4 new arrays per module regardless of if they are needed. Instead of falling back to [] we could set to null, and in setHookFunction below you could check for null and create the array when needed.
There was a problem hiding this comment.
I'm unsure which is better. I went this route so that we don't have to have as much branching in the logic and I doubt it will have much impact on test performance.
src/test.js
Outdated
| var promise, | ||
| test = this; | ||
| queueHook( hook, hookName, hookOwner ) { | ||
| const test = this; |
There was a problem hiding this comment.
Can we use arrow functions below instead? I am unsure if we transpile though...
There was a problem hiding this comment.
Good suggestion. I can do that, but will need to save it to a variable so that we get a name in the stack trace for better debuggability.
trentmwillis
force-pushed
the
multiple-hooks
branch
from
d9eea7e
to
c2ee12b
Compare
|
Updated to address comments.
Yep, I added some comments but nothing too lengthy. The entire
Yeah, the other tests in the file should handle the other features. We can add additional tests later if bugs are found. |
| @@ -199,10 +199,12 @@ QUnit.test( "makes a car", function( assert ) { | |||
|
|
|||
| --- | |||
|
|
|||
| Hooks stack on nested modules | |||
| `before`/`beforeEach` hooks queue on nested modules. `after`/`afterEach` hooks stack on nested modules. | |||
There was a problem hiding this comment.
This makes sense for those familiar with the data structures, but it might not be accessible to software engineers who don't have a strong background in data structures. Maybe this could be reworded to say outer and earlier-declared before/beforeEach functions run before inner and later-declared ones, and the reverse for after/afterEach? (Though I'll admit what I've proposed isn't great, either.)
There was a problem hiding this comment.
Further up in the document it says:
Additionally, any hook callbacks on a parent module will wrap the hooks on a nested module. In other words, before and beforeEach callbacks will form a queue while the afterEach and after callbacks will form a stack.
So I'm just trying to keep consistent messaging. If you feel strongly, I can reword it now, but as mentioned before I do think we need to rework the majority of this doc.
There was a problem hiding this comment.
No, I just missed that. I agree this needs to be reworked later but also agree that consistent messaging now is good.
|
@qunitjs/qunit-team I'm going to leave this open for another 24 hours in case anyone else wants to review. I know there were some differing opinions on the order of execution for the hooks, so I don't want to jump the gun on merging. That said, I'm fairly confident in this ordering since it is consistent with the current behavior for nested modules. |
| setHookFromEnvironment( hooks, testEnvironment, "afterEach" ); | ||
| setHookFromEnvironment( hooks, testEnvironment, "after" ); | ||
|
|
||
| function setHookFromEnvironment( hooks, environment, name ) { |
There was a problem hiding this comment.
Not sure the hooks and environment parameters add value in this case. Is this is a V8-optimisation perhaps?
There was a problem hiding this comment.
Unsure what you mean as both parameters are used in the function. Unless you're referring to the function itself, which is really just here to avoid repeating the same code for each hook.
There was a problem hiding this comment.
Yeah, but I was suggesting to have setHookFromEnvironment take only one parameter, name. Since the rest is just here in scope?
trentmwillis
force-pushed
the
multiple-hooks
branch
from
c2ee12b
to
a593f28
Compare
Fixes #977.
When using the nested module pattern, you can now define a multiple of each hook (
before/beforeEach/afterEach/after).before/beforeEachhooks are run FIFO order andafter/afterEachare run in LIFO order. This is consistent with the behavior already exhibited by those hooks when nested in modules.cc @rwjblue (who I believe is interested in this feature for ember-qunit).