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.

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

feat: on boot draft #1679

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: on boot draft #1679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec7b24a
feat: on boot draft
OlaviSau Jun 7, 2024
3956674
Merge branch 'vikejs:main' into on-boot
OlaviSau Jul 2, 2024
5015a63
Merge branch 'vikejs:main' into on-boot
OlaviSau Jul 6, 2024
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
20 changes: 17 additions & 3 deletions vike/client/client-routing-runtime/createPageContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import { getPageContextUrlComputed } from '../../shared/getPageContextUrlCompute
import { getPageFilesAll } from '../../shared/getPageFiles.js'
import { loadPageRoutes } from '../../shared/route/loadPageRoutes.js'
import { getBaseServer } from './getBaseServer.js'
import { assert, isBaseServer, PromiseType, getGlobalObject, objectAssign } from './utils.js'
import { assert, isBaseServer, PromiseType, getGlobalObject, objectAssign, isObject } from './utils.js'
const globalObject = getGlobalObject<{
onBootResult?: Record<string, unknown>,
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this goes in the right direction: we want to call onBoot() only once and cache its result "forever".

pageFilesData?: PromiseType<ReturnType<typeof getPageFilesAll>>
}>('createPageContext.ts', {})

Expand All @@ -14,15 +15,16 @@ async function createPageContext(urlOriginal: string) {
globalObject.pageFilesData = await getPageFilesAll(true)
}
const { pageFilesAll, allPageIds, pageConfigs, pageConfigGlobal } = globalObject.pageFilesData
const { pageRoutes, onBeforeRouteHook } = await loadPageRoutes(
const { pageRoutes, onBeforeRouteHook, onBootHook } = await loadPageRoutes(
pageFilesAll,
pageConfigs,
pageConfigGlobal,
allPageIds
)
const baseServer = getBaseServer()
assert(isBaseServer(baseServer))
const pageContext = {

let pageContext = {
urlOriginal,
_objectCreatedByVike: true,
_urlHandler: null,
Expand All @@ -35,6 +37,18 @@ async function createPageContext(urlOriginal: string) {
_pageRoutes: pageRoutes,
_onBeforeRouteHook: onBeforeRouteHook
}

if (!globalObject.onBootResult && onBootHook) {
globalObject.onBootResult = await onBootHook.hookFn(pageContext) as Record<string, unknown>
}

if (isObject(globalObject.onBootResult)) {
pageContext = {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use Object.assign() instead of re-creating a new object.

...pageContext,
...globalObject.onBootResult
}
}

const pageContextUrlComputed = getPageContextUrlComputed(pageContext)
objectAssign(pageContext, pageContextUrlComputed)
return pageContext
Expand Down
2 changes: 1 addition & 1 deletion vike/node/plugin/plugins/importUserCode/v1-design/getVikeConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ async function getGlobalConfigs(
)
const configValueSource = sources[0]
if (!configValueSource) return
if (configName === 'onBeforeRoute' || configName === 'onPrerenderStart') {
if (['onBeforeRoute', 'onPrerenderStart', 'onBoot'].includes(configName)) {
assert(!('value' in configValueSource))
pageConfigGlobal.configValueSources[configName] = [configValueSource]
} else {
Expand Down
4 changes: 4 additions & 0 deletions vike/node/plugin/plugins/importUserCode/v1-design/getVikeConfig/configDefinitionsBuiltIn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ const configDefinitionsBuiltIn: ConfigDefinitionsBuiltIn = {
}

type ConfigNameGlobal =
| 'onBoot'
| 'onPrerenderStart'
| 'onBeforeRoute'
| 'prerender'
Expand All @@ -206,6 +207,9 @@ const configDefinitionsBuiltInGlobal: Record<ConfigNameGlobal, ConfigDefinitionI
onBeforeRoute: {
env: { server: true, client: 'if-client-routing', eager: true }
},
onBoot: {
env: { server: true, client: true, eager: true }
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 not sure, but I don't think we need eager.

Actually, we need eager if we make onBoot() non-global and cumulative (see my sibling comment).

},
prerender: {
env: { config: true }
},
Expand Down
8 changes: 8 additions & 0 deletions vike/node/runtime/renderPage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ async function renderPageAndPrepare(
// From now on, renderContext.pageConfigs contains all the configuration data; getVikeConfig() isn't called anymore for this request
}

if (renderContext.onBootHook) {
const onBootResult = await renderContext.onBootHook?.hookFn(pageContextInit) as Record<string, unknown>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should call onBoot() at request-time? My thinking was to call onBoot() exactly once and as soon as possible whenever its code is loaded and then cache its result.

pageContextInit = {
...pageContextInit,
...onBootResult
};
}

// Check Base URL
{
const pageContextHttpResponse = checkBaseUrl(pageContextInit, httpRequestId)
Expand Down
6 changes: 4 additions & 2 deletions vike/node/runtime/renderPage/renderPageAlreadyRouted.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ type RenderContext = {
allPageIds: string[]
pageRoutes: PageRoutes
onBeforeRouteHook: Hook | null
onBootHook: Hook | null
}
// TODO: remove getRenderContext() in favor of getGlobalObject() + reloadGlobalContext()
// TODO: impl GlobalNodeContext + GlobalClientContext + GloablContext, and use GlobalContext instead of RenderContext
Expand All @@ -261,7 +262,7 @@ async function getRenderContext(): Promise<RenderContext> {
false,
globalContext.isProduction
)
const { pageRoutes, onBeforeRouteHook } = await loadPageRoutes(
const { pageRoutes, onBeforeRouteHook, onBootHook } = await loadPageRoutes(
pageFilesAll,
pageConfigs,
pageConfigGlobal,
Expand All @@ -278,7 +279,8 @@ async function getRenderContext(): Promise<RenderContext> {
pageConfigGlobal,
allPageIds: allPageIds,
pageRoutes,
onBeforeRouteHook
onBeforeRouteHook,
onBootHook
}
return renderContext
}
24 changes: 22 additions & 2 deletions vike/shared/page-configs/Config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export type { OnBeforePrerenderStartAsync }
export type { OnBeforePrerenderStartSync }
export type { OnBeforeRenderAsync }
export type { OnBeforeRenderSync }
export type { OnBoot }
export type { OnBootSync }
export type { OnBeforeRouteAsync }
export type { OnBeforeRouteSync }
export type { OnHydrationEndAsync }
Expand Down Expand Up @@ -51,12 +53,12 @@ type HookNamePage =
| 'onRenderClient'
| 'guard'
| 'data'
type HookNameGlobal = 'onBeforePrerender' | 'onBeforeRoute' | 'onPrerenderStart'
type HookNameGlobal = 'onBeforePrerender' | 'onBeforeRoute' | 'onPrerenderStart' | 'onBoot'
// v0.4 design TODO/v1-release: remove
type HookNameOldDesign = 'render' | 'prerender'

type ConfigNameBuiltIn =
| Exclude<keyof Config, keyof ConfigVikeUserProvided | 'onBeforeRoute' | 'onPrerenderStart'>
| Exclude<keyof Config, keyof ConfigVikeUserProvided | 'onBeforeRoute' | 'onPrerenderStart' | 'onBoot'>
| 'prerender'
| 'clientEntryLoaded'
| 'onBeforeRenderEnv'
Expand Down Expand Up @@ -131,6 +133,18 @@ type OnBeforeRenderAsync = (
* https://vike.dev/onBeforeRender
*/
type OnBeforeRenderSync = (pageContext: PageContextServer) => { pageContext: Partial<Vike.PageContext> } | void
/** Hook called when Vike is first initialized.
*
* https://vike.dev/onBoot
*/
type OnBoot = (
Copy link
Member

Choose a reason for hiding this comment

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

Let's name it OnBootAsync to keep it consistent with the other type names.

pageContext: PageContextServer
) => Promise<{ pageContext: Partial<Vike.PageContext> } | void>
/** Hook called when Vike is first initialized.
*
* https://vike.dev/onBoot
*/
type OnBootSync = (pageContext: PageContextServer) => { pageContext: Partial<Vike.PageContext> } | void
/** Hook called before the URL is routed to a page.
*
* https://vike.dev/onBeforeRoute
Expand Down Expand Up @@ -349,6 +363,12 @@ type ConfigBuiltIn = {
*/
onBeforePrerenderStart?: OnBeforePrerenderStartAsync | OnBeforePrerenderStartSync | ImportString

/** Hook called when Vike is first initialized.
*
* https://vike.dev/onBoot
*/
onBoot?: OnBoot | OnBootSync | ImportString

/** Hook called before the URL is routed to a page.
*
* https://vike.dev/onBeforeRoute
Expand Down
14 changes: 8 additions & 6 deletions vike/shared/route/loadPageRoutes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ async function loadPageRoutes(
pageConfigs: PageConfigRuntime[],
pageConfigGlobal: PageConfigGlobalRuntime,
allPageIds: string[]
): Promise<{ pageRoutes: PageRoutes; onBeforeRouteHook: null | Hook }> {
): Promise<{ pageRoutes: PageRoutes; onBeforeRouteHook: null | Hook; onBootHook: null | Hook }> {
await Promise.all(pageFilesAll.filter((p) => p.fileType === '.page.route').map((p) => p.loadFile?.()))
const { onBeforeRouteHook, filesystemRoots } = getGlobalHooks(pageFilesAll, pageConfigs, pageConfigGlobal)
const { onBootHook, onBeforeRouteHook, filesystemRoots } = getGlobalHooks(pageFilesAll, pageConfigs, pageConfigGlobal)
const pageRoutes = getPageRoutes(filesystemRoots, pageFilesAll, pageConfigs, allPageIds)
return { pageRoutes, onBeforeRouteHook }
return { pageRoutes, onBeforeRouteHook, onBootHook }
}

function getPageRoutes(
Expand Down Expand Up @@ -175,12 +175,14 @@ function getGlobalHooks(
pageConfigGlobal: PageConfigGlobalRuntime
): {
onBeforeRouteHook: null | Hook
onBootHook: null | Hook
filesystemRoots: null | FilesystemRoot[]
} {
// V1 Design
if (pageConfigs.length > 0) {
const hook = getHookFromPageConfigGlobal(pageConfigGlobal, 'onBeforeRoute')
return { onBeforeRouteHook: hook, filesystemRoots: null }
const onBootHook = getHookFromPageConfigGlobal(pageConfigGlobal, 'onBoot')
const onBeforeRouteHook = getHookFromPageConfigGlobal(pageConfigGlobal, 'onBeforeRoute')
return { onBootHook, onBeforeRouteHook, filesystemRoots: null }
Copy link
Member

Choose a reason for hiding this comment

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

It's a good question whether onBoot() should be global.

Maybe we can make onBoot() non-global and cumulative. In other words: the result of the various onBoot() hooks are cached and only the results of the relevant onBoot() hooks are applied each time Vike creates a new pageContext object.

But this means that onBoot() can be called later upon navigating to a new page and onBoot() is a misnomer then. Ah, no, actually if make onBoot() eager then we can call all onBoot() hooks at boot time.

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 actually leaning more towards making onBoot() global as you suggested.

The thing is that, if we don't make it global, then we have to resolve routing before calling onBoot(), which is somewhat contradictory as an idea of onBoot() is to call it as early as possible and before any request is made to the server.

If a user wants a per-page hook, then he can use onBeforeRender(). (So far I don't think we need to introduce a second hook onRequest().)

WDYT?

}

// Old design
Expand Down Expand Up @@ -221,7 +223,7 @@ function getGlobalHooks(
}
})

return { onBeforeRouteHook, filesystemRoots }
return { onBootHook: null, onBeforeRouteHook, filesystemRoots }
}

function dirname(filePath: string): string {
Expand Down