refactor: 💡 Add options to the "call" middleware #17
Conversation
| import { isHydrate } from '../../../utils' | ||
|
|
||
| export default (name, route, req) => { | ||
| export default (name, route, req, { runOnServer, runOnHydrate }) => { |
There was a problem hiding this comment.
Do you think its a good idea to make the default behaviour that it runs on the server and not hydrate? Seems like usually people want callbacks to happen at least at some point on initial load.
There was a problem hiding this comment.
I can't really visualize the use-case for these from the end-users POV, as I haven't used SSR personally. However I do think that skipOnServer (+ reversing logic in shouldCall) is a better API than runOnServer = true in that case.
There was a problem hiding this comment.
I like that both options have the same positive polarity (runOnX + skipOnY I think is more confusing) but potentially one could default to true?
There was a problem hiding this comment.
I agree that it's confusing, but implicit true seems even worse. :-)
Undefined is false in JS so it's easy to see when an option is not set. If we change that, it suddenly becomes hard to know whether it's required to say "fooFlag: false" or "barFlag: true", and it somehow becomes a tristate.
Is there a third option?
There was a problem hiding this comment.
I forget which babel plugin its in, but I think I've seen code like
(name, route, req, { runOnServer = true, runOnHydrate = false }) => {}There was a problem hiding this comment.
Don't worry though, I think its fine the way it is, simple and clear.
|
Howdy, beeen out the loop for a while. Ill review this tomorrow. Thank you for this!!! |
|
I've rebased this branch on master but it seems that that is not enough to make Snyk happy... |
"call" now needs the new options "runOnServer", "runOnHydrate" to be passed explicitly, rather than using hard-coded logic to determine where to run. "runOnClientRender" has not been added since there's currently no use-case for skipping client render. This aims to clarify when each callback runs.
|
Turns out the tests were failing because of jestjs/jest#8069 Travis updated their node now 🎉 |
"call" now needs the new options "runOnServer", "runOnHydrate" to be
passed explicitly, rather than using hard-coded logic to determine where
to run.
"runOnClientRender" has not been added since there's currently no
use-case for skipping client render.
This aims to clarify when each callback runs.