Skip to content

Commit

Permalink
fix: make .client.js and .server.js work for dynamic imports (fix #1861
Browse files Browse the repository at this point in the history
…) (#1884)
  • Loading branch information
brillout authored Sep 19, 2024
1 parent e62abbd commit f7e91ba
Show file tree
Hide file tree
Showing 8 changed files with 186 additions and 93 deletions.
15 changes: 12 additions & 3 deletions .github/workflows/ci/getTestJobs.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -31,25 +31,34 @@ async function getTestJobs() {
const specFiles = projectFiles.filter((file) => file.includes('.spec.'))
const testFiles = projectFiles.filter((file) => file.includes('.test.'))

const linux_nodeOld = {
os: 'ubuntu-latest',
node_version: '18'
}
const windows_nodeOld = {
os: 'windows-latest',
node_version: '18'
}

/** @type { Job[] } */
let jobs = [
{
jobName: 'Vitest (unit tests)',
jobCmd: 'pnpm exec vitest run --project unit',
jobTestFiles: specFiles,
jobSetups: [{ os: 'ubuntu-latest', node_version: '18' }]
jobSetups: [linux_nodeOld]
},
{
jobName: 'Vitest (E2E tests)',
jobCmd: 'pnpm exec vitest run --project e2e',
jobTestFiles: specFiles,
jobSetups: [{ os: 'ubuntu-latest', node_version: '18' }]
jobSetups: [linux_nodeOld, windows_nodeOld]
},
// Check TypeScript types
{
jobName: 'TypeScript',
jobCmd: 'pnpm exec test-types',
jobSetups: [{ os: 'ubuntu-latest', node_version: '18' }]
jobSetups: [linux_nodeOld]
},
// E2e tests
...(await crawlE2eJobs(testFiles))
Expand Down
16 changes: 16 additions & 0 deletions test/misc/pages/dynamic-import-file-env/+Page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react'
import { isBrowser } from './isBrowser'

if (isBrowser) {
import('./hello.client')
} else {
import('./hello.server')
}

export function Page() {
return (
<>
<p>Dyanmic import() of .client.js and .server.js</p>
</>
)
}
3 changes: 3 additions & 0 deletions test/misc/pages/dynamic-import-file-env/hello.client.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { isBrowser } from './isBrowser'
if (!isBrowser) throw new Error("I shouldn't be imported from the server")
console.log('hello from client')
3 changes: 3 additions & 0 deletions test/misc/pages/dynamic-import-file-env/hello.server.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { isBrowser } from './isBrowser'
if (isBrowser) throw new Error("I shouldn't be imported from the client")
console.log('hello from server')
1 change: 1 addition & 0 deletions test/misc/pages/dynamic-import-file-env/isBrowser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const isBrowser = typeof window === 'object'
17 changes: 16 additions & 1 deletion test/misc/testRun.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export { testRun }

import { test, expect, fetch, fetchHtml, page, getServerUrl, autoRetry, sleep } from '@brillout/test-e2e'
import { test, expect, fetch, fetchHtml, page, getServerUrl, autoRetry, sleep, expectLog } from '@brillout/test-e2e'
import { expectUrl, testCounter } from '../utils'
import { testRun as testRunClassic } from '../../examples/react-minimal/.testRun'
import fs from 'fs'
Expand All @@ -18,6 +18,7 @@ function testRun(cmd: 'npm run dev' | 'npm run preview' | 'npm run prod') {
testPrerenderSettings()
testRedirectMailto()
testNavigateEarly()
testDynamicImportFileEnv()
testNestedConfigWorkaround()
testHistoryPushState()
}
Expand Down Expand Up @@ -98,6 +99,20 @@ function testNavigateEarly() {
})
}

function testDynamicImportFileEnv() {
test('Dyanmic import() of .client.js and .server.js', async () => {
await page.goto(getServerUrl() + '/dynamic-import-file-env')
expect(await page.textContent('body')).toContain('Dyanmic import() of .client.js and .server.js')
expectLog('hello from server', (log) => log.logSource === 'stdout')
await autoRetry(
() => {
expectLog('hello from client', (log) => log.logSource === 'Browser Log')
},
{ timeout: 5000 }
)
})
}

function testNestedConfigWorkaround() {
// See comment in /test/misc/pages/+config.ts
test('Nested config workaround', async () => {
Expand Down
7 changes: 6 additions & 1 deletion test/preload/testRun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,13 @@ async function render(urlOriginal: '/' | '/preload-disabled' | '/preload-images'
return { body, earlyHints }
}

const workspaceRoot = path.join(__dirname, '..', '..')
const workspaceRoot = getWorkspaceRoot()
function stabilizePaths(str: string): string {
str = str.replaceAll(workspaceRoot, '/$ROOT')
return str
}
function getWorkspaceRoot() {
let workspaceRoot = path.join(__dirname, '..', '..').split('\\').join('/')
if (!workspaceRoot.startsWith('/')) workspaceRoot = '/' + workspaceRoot
return workspaceRoot
}
217 changes: 129 additions & 88 deletions vike/node/plugin/plugins/fileEnv.ts
Original file line number Diff line number Diff line change
@@ -1,114 +1,155 @@
export { fileEnv }

// Implementation for https://vike.dev/file-env
// Alternative implementations:
// - Remix: https://github.com/remix-run/remix/blob/0e542779499b13ab9291cf20cd5e6b43e2905151/packages/remix-dev/vite/plugin.ts#L1504-L1594
// - SvelteKit: https://github.com/sveltejs/kit/blob/6ea7abbc2f66e46cb83ff95cd459a5f548cb7e1e/packages/kit/src/exports/vite/index.js#L383-L401

import type { Plugin, ResolvedConfig } from 'vite'
import { assert, assertUsage, assertWarning, capitalizeFirstLetter } from '../utils.js'
import type { Plugin, ResolvedConfig, ViteDevServer } from 'vite'
import { assert, assertUsage, assertWarning, capitalizeFirstLetter, joinEnglish } from '../utils.js'
import { extractAssetsRE } from './extractAssetsPlugin.js'
import { extractExportNamesRE } from './extractExportNamesPlugin.js'
import pc from '@brillout/picocolors'
import { getModuleFilePathAbsolute } from '../shared/getFilePath.js'
import { sourceMapRemove } from '../shared/rollupSourceMap.js'
import { getExportNames } from '../shared/parseEsModule.js'

function fileEnv(): Plugin {
let config: ResolvedConfig
let isDev = false
let viteDevServer: ViteDevServer | undefined
return {
name: 'vike:fileEnv',
// - We need to set `enforce: 'pre'` because, otherwise, the resolvedId() hook of Vite's internal plugin `vite:resolve` is called before and it doesn't seem to call `this.resolve()` which means that the resolveId() hook below is never called.
// - Vite's `vite:resolve` plugin: https://github.com/vitejs/vite/blob/d649daba7682791178b711d9a3e44a6b5d00990c/packages/vite/src/node/plugins/resolve.ts#L105
// - It's actually a good thing if the resolveId() hook below is the first one to be called because it doesn't actually resolve any ID, so all other resolveId() hooks will be called as normal. And with `this.resolve()` we get the information we want from all other resolvedId() hooks.
// - Path aliases are already resolved, even when using `enforce: 'pre'`. For example:
// ```js
// // /pages/index/+Page.tsx
//
// // The value of `source` is `/home/rom/code/vike/examples/path-aliases/components/Counter` (instead of `#root/components/Counter`)
// // The value of `importer` is `/home/rom/code/vike/examples/path-aliases/pages/index/+Page.tsx`
// import { Counter } from '#root/components/Counter'
// ```
enforce: 'pre',
resolveId: {
/* I don't know why, but path aliases aren't resolved anymore when setting `order: 'pre'`. (In principle, I'd assume that `this.resolve()` would resolve the alias but it doesn't.)
order: 'pre',
*/
async handler(source, importer, options) {
// It seems like Vite's scan doesn't apply transformers. (We need the `.telefunc.js` transformer to apply for our analysis to be correct.)
// @ts-expect-error Vite's type is wrong
if (options.scan) return
load(id, options) {
// In build, we use generateBundle() instead of the load() hook. Using load() works for dynamic imports in dev thanks to Vite's lazy transpiling, but it doesn't work in build because Rollup transpiles any dynamically imported module even if it's never actually imported.
if (!viteDevServer) return
if (skip(id)) return
const moduleInfo = viteDevServer.moduleGraph.getModuleById(id)
assert(moduleInfo)
const importers: string[] = Array.from(moduleInfo.importers)
.map((m) => m.id)
.filter((id) => id !== null)
assertFileEnv(
id,
!!options?.ssr,
importers,
// In dev, we only show a warning because we don't want to disrupt when the user plays with settings such as [ssr](https://vike.dev/ssr).
true
)
},
// In production, we have to use transform() to replace modules with a runtime error because generateBundle() doesn't work for dynamic imports. In production, dynamic imports can only be verified at runtime.
async transform(code, id, options) {
// In dev, only using load() is enough as it also works for dynamic imports (see sibling comment).
if (viteDevServer) return
if (skip(id)) return
const isServerSide = !!options?.ssr
if (!isWrongEnv(id, isServerSide)) return
const { importers } = this.getModuleInfo(id)!
// Throwing a verbose error doesn't waste client-side KBs as dynamic imports are code splitted.
const errMsg = getErrorMessage(id, isServerSide, importers, false, true)
// We have to inject empty exports to avoid Rollup complaining about missing exports, see https://gist.github.com/brillout/5ea45776e65bd65100a52ecd7bfda3ff
const { exportNames } = await getExportNames(code)
return sourceMapRemove(
[
`throw new Error(${JSON.stringify(errMsg)});`,
...exportNames.map((name) =>
name === 'default' ? 'export default undefined;' : `export const ${name} = undefined;`
)
].join('\n')
)
},
generateBundle() {
Array.from(this.getModuleIds())
.filter((id) => !skip(id))
.forEach((moduleId) => {
const { importers, dynamicImporters } = this.getModuleInfo(moduleId)!
if (importers.length === 0) {
// Dynamic imports can only be verified at runtime
assert(dynamicImporters.length > 0)
return
}
assertFileEnv(moduleId, !!config.build.ssr, importers, false)
})
},
configResolved(config_) {
config = config_
},
configureServer(viteDevServer_) {
viteDevServer = viteDevServer_
}
}

// TODO/v1-release: remove
if (extractAssetsRE.test(source) || extractExportNamesRE.test(source)) return
function assertFileEnv(
moduleId: string,
isServerSide: boolean,
importers: string[] | readonly string[],
onlyWarn: boolean
) {
if (!isWrongEnv(moduleId, isServerSide)) return
const errMsg = getErrorMessage(moduleId, isServerSide, importers, onlyWarn, false)
if (onlyWarn) {
assertWarning(false, errMsg, { onlyOnce: true })
} else {
assertUsage(false, errMsg)
}
}

// Seems like Vite is doing some funky stuff here.
if (importer?.endsWith('.html')) return
function getErrorMessage(
moduleId: string,
isServerSide: boolean,
importers: string[] | readonly string[],
onlyWarn: boolean,
noColor: boolean
) {
const modulePath = getModulePath(moduleId)

const resolved = await this.resolve(source, importer, {
// Needed for old Vite plugins: https://vitejs.dev/guide/migration#rollup-4:~:text=For%20Vite%20plugins%2C%20this.resolve%20skipSelf%20option%20is%20now%20true%20by%20default.
skipSelf: true,
...options
})
// resolved is null when import path is erroneous and doesn't actually point to a file
if (!resolved) return
const moduleId = resolved.id
const modulePath = moduleId.split('?')[0]!
const envActual = isServerSide ? 'server' : 'client'
const envExpect = isServerSide ? 'client' : 'server'

// `.server.js` and `.client.js` should only apply to user files
if (modulePath.includes('/node_modules/')) return
let errMsg: string
let modulePathPretty = getModuleFilePathAbsolute(modulePath, config)
if (!noColor) {
const suffix = getSuffix(envExpect)
modulePathPretty = modulePathPretty.replaceAll(suffix, pc.bold(suffix))
}
errMsg = `${capitalizeFirstLetter(
envExpect
)}-only file ${modulePathPretty} (https://vike.dev/file-env) imported on the ${envActual}-side`

// TODO/v1-release: remove
// - I don't remember exactly, but I think I've added `TODO/v1-release: remove` because I vaguely remember that we can remove this after we remove the 0.4 design.
if (modulePath.endsWith('.css')) return
if (importers.length > 0) {
const importPaths = importers.map((importer) => getModuleFilePathAbsolute(importer, config))
errMsg += ` by ${joinEnglish(importPaths, 'and')}`
}

const isServerSide = options?.ssr
const envActual = isServerSide ? 'server' : 'client'
const envExpect = isServerSide ? 'client' : 'server'
const suffix = `.${envExpect}.` as const
if (onlyWarn) {
errMsg += ' and, therefore, Vike will prevent building your app for production.'
}

// Everything is good
if (!modulePath.includes(suffix)) return
return errMsg
}

// Show error message
let errMsg: string
let modulePathPretty = getModuleFilePathAbsolute(moduleId, config)
modulePathPretty = modulePathPretty.replaceAll(suffix, pc.bold(suffix))
errMsg = `${capitalizeFirstLetter(
envExpect
)}-only file ${modulePathPretty} (https://vike.dev/file-env) imported on the ${envActual}-side`
function isWrongEnv(moduleId: string, isServerSide: boolean): boolean {
const modulePath = getModulePath(moduleId)
const suffixWrong = getSuffix(isServerSide ? 'client' : 'server')
return modulePath.includes(suffixWrong)
}

if (
importer &&
// Don't show Vike's virtual modules that import the entry plus files such as /pages/about/+Page.js
!importer.includes('virtual:vike:') &&
// I don't know why and who sets importer to '<stdin>' (I guess Vite?)
importer !== '<stdin>'
) {
const importerPath = getModuleFilePathAbsolute(importer, config)
errMsg += ` by ${importerPath}`
}
function skip(id: string): boolean {
// TODO/v1-release: remove
if (extractAssetsRE.test(id) || extractExportNamesRE.test(id)) return true
if (!id.includes(getSuffix('client')) && !id.includes(getSuffix('server'))) return true
if (getModulePath(id).endsWith('.css')) return true
// Apply `.server.js` and `.client.js` only to user files
if (id.includes('/node_modules/')) return true
// Only user files
if (!id.startsWith(config.root)) return true
return false
}

if (isDev) {
errMsg += ' and, therefore, Vike will prevent building your app for production.'
assertWarning(false, errMsg, { onlyOnce: true })
} else {
assertUsage(false, errMsg)
}
}
},
configResolved(config_) {
config = config_
},
configureServer() {
isDev = true
},
// Ensure this plugin works
transform(_code, id, options): void {
if (isDev) return
// TODO/v1-release: remove
if (extractAssetsRE.test(id) || extractExportNamesRE.test(id)) return
if (id.split('?')[0]!.endsWith('.css')) return
function getSuffix(env: 'client' | 'server') {
return `.${env}.` as const
}

const isServerSide = options?.ssr
const envWrong = isServerSide ? 'client' : 'server'
assert(!id.includes(`.${envWrong}.`))
}
function getModulePath(moduleId: string) {
return moduleId.split('?')[0]!
}
}

0 comments on commit f7e91ba

Please sign in to comment.