Skip to content

refactor: 💡 Add options to the "call" middleware #17

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

Merged
merged 1 commit into from
Mar 27, 2019
Merged

refactor: 💡 Add options to the "call" middleware #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions packages/rudy/src/core/createRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @flow
import qs from 'qs'
import { createSelector } from '@respond-framework/utils'
import type {
Options,
Store,
Expand All @@ -22,8 +23,6 @@ import {
onError as defaultOnError,
} from '../utils'

import { createSelector } from '@respond-framework/utils'

import {
serverRedirect,
pathlessRoute,
Expand All @@ -42,14 +41,16 @@ export default (
anonymousThunk,
pathlessRoute('thunk'),
transformAction, // pipeline starts here
// Hydrate: skip callbacks called on server to produce initialState (beforeEnter, thunk, etc)
// Server: don't allow client-centric callbacks (onEnter, onLeave, beforeLeave)
call('beforeLeave', { prev: true }),
call('beforeEnter'),
call('beforeEnter', { runOnServer: true }),
enter,
changePageTitle(),
call('onLeave', { prev: true }),
call('onEnter'),
call('thunk', { cache: true }),
call('onComplete'),
call('onEnter', { runOnHydrate: true }),
call('thunk', { cache: true, runOnServer: true }),
call('onComplete', { runOnServer: true }),
],
) => {
const {
Expand Down Expand Up @@ -82,7 +83,9 @@ export default (
const has = (name: string) => wares[name]
const ctx = { busy: false }
const api = { routes, history, options, register, has, ctx }
const onError = call('onError')(api)
const onError = call('onError', { runOnServer: true, runOnHydrate: true })(
api,
)
const nextPromise = options.compose(
middlewares,
api,
Expand Down
10 changes: 4 additions & 6 deletions packages/rudy/src/middleware/call/utils/shouldCall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
// @flow
import type { LocationState } from '../../../flow-types'
import { isServer } from '@respond-framework/utils'
import type { LocationState } from '../../../flow-types'
import { isHydrate } from '../../../utils'

export default (name, route, req) => {
export default (name, route, req, { runOnServer, runOnHydrate }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that both options have the same positive polarity (runOnX + skipOnY I think is more confusing) but potentially one could default to true?

Copy link
Contributor Author

@toerndev toerndev Feb 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I forget which babel plugin its in, but I think I've seen code like

(name, route, req, { runOnServer = true, runOnHydrate = false }) => {}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't worry though, I think its fine the way it is, simple and clear.

if (!route[name] && !req.options[name]) return false

// skip callbacks (beforeEnter, thunk, etc) called on server, which produced initialState
if (isHydrate(req) && !/onEnter|onError/.test(name)) return false
if (isHydrate(req) && !runOnHydrate) return false

// dont allow these client-centric callbacks on the server
if (isServer() && /onEnter|Leave/.test(name)) return false
if (isServer() && !runOnServer) return false

return allowBoth
}
Expand Down
4 changes: 3 additions & 1 deletion packages/rudy/src/middleware/pathlessRoute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ export default (...names) => (api) => {
names[0] = names[0] || 'thunk'
names[1] = names[1] || 'onComplete'

const middlewares = names.map((name) => call(name, { skipOpts: true }))
const middlewares = names.map((name) =>
call(name, { runOnServer: true, skipOpts: true }),
)

const pipeline = api.options.compose(
middlewares,
Expand Down