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.

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: add list of known js source extensions to config #3828

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
87 changes: 57 additions & 30 deletions packages/vite/src/node/__tests__/utils.spec.ts
Original file line number Diff line number Diff line change
@@ -1,40 +1,67 @@
import { injectQuery, isWindows } from '../utils'
import { injectQuery, isJSRequest, isWindows } from '../utils'

if (isWindows) {
// this test will work incorrectly on unix systems
test('normalize windows path', () => {
expect(injectQuery('C:\\User\\Vite\\Project', 'direct')).toEqual(
'C:/User/Vite/Project?direct'
describe('injectQuery', () => {
if (isWindows) {
// this test will work incorrectly on unix systems
test('normalize windows path', () => {
expect(injectQuery('C:\\User\\Vite\\Project', 'direct')).toEqual(
'C:/User/Vite/Project?direct'
)
})
}

test('path with multiple spaces', () => {
expect(injectQuery('/usr/vite/path with space', 'direct')).toEqual(
'/usr/vite/path with space?direct'
)
})
}

test('path with multiple spaces', () => {
expect(injectQuery('/usr/vite/path with space', 'direct')).toEqual(
'/usr/vite/path with space?direct'
)
})
test('path with multiple % characters', () => {
expect(injectQuery('/usr/vite/not%20a%20space', 'direct')).toEqual(
'/usr/vite/not%20a%20space?direct'
)
})

test('path with multiple % characters', () => {
expect(injectQuery('/usr/vite/not%20a%20space', 'direct')).toEqual(
'/usr/vite/not%20a%20space?direct'
)
})
test('path with %25', () => {
expect(injectQuery('/usr/vite/%25hello%25', 'direct')).toEqual(
'/usr/vite/%25hello%25?direct'
)
})

test('path with %25', () => {
expect(injectQuery('/usr/vite/%25hello%25', 'direct')).toEqual(
'/usr/vite/%25hello%25?direct'
)
})
test('path with unicode', () => {
expect(injectQuery('/usr/vite/東京', 'direct')).toEqual(
'/usr/vite/東京?direct'
)
})
patak-dev marked this conversation as resolved.
Show resolved Hide resolved

test('path with unicode', () => {
expect(injectQuery('/usr/vite/東京', 'direct')).toEqual(
'/usr/vite/東京?direct'
)
test('path with unicode, space, and %', () => {
expect(injectQuery('/usr/vite/東京 %20 hello', 'direct')).toEqual(
'/usr/vite/東京 %20 hello?direct'
)
})
})

test('path with unicode, space, and %', () => {
expect(injectQuery('/usr/vite/東京 %20 hello', 'direct')).toEqual(
'/usr/vite/東京 %20 hello?direct'
)
describe('isJSRequest', () => {
const knownJsSrcExtensions = ['.js', '.ts']
test.each([
['', true], // bare imports are js
['.js', true],
['.ts', true],
['.x', false], // not in extensions list => false
['/', false] // directory => false
])('path ending with "%s" returns %s', (suffix, expected) => {
const path = `/x/y/foo${suffix}`
expect(isJSRequest(path, knownJsSrcExtensions)).toBe(expected)
// also tests combinations of querystring and hash, must be the same result
expect(isJSRequest(`${path}?foo=.js`, knownJsSrcExtensions)).toBe(expected)
expect(isJSRequest(`${path}#.js`, knownJsSrcExtensions)).toBe(expected)
expect(isJSRequest(`${path}?foo=.js#.js`, knownJsSrcExtensions)).toBe(
expected
)
expect(isJSRequest(`${path}?foo=.x`, knownJsSrcExtensions)).toBe(expected)
expect(isJSRequest(`${path}#.x`, knownJsSrcExtensions)).toBe(expected)
expect(isJSRequest(`${path}?foo=.x#.x`, knownJsSrcExtensions)).toBe(
expected
)
})
})
36 changes: 34 additions & 2 deletions packages/vite/src/node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ import { ESBuildOptions } from './plugins/esbuild'
import dotenv from 'dotenv'
import dotenvExpand from 'dotenv-expand'
import { Alias, AliasOptions } from 'types/alias'
import { CLIENT_DIR, DEFAULT_ASSETS_RE } from './constants'
import {
CLIENT_DIR,
DEFAULT_ASSETS_RE,
DEFAULT_KNOWN_JS_SRC_EXTENSIONS
} from './constants'
import {
InternalResolveOptions,
ResolveOptions,
Expand Down Expand Up @@ -129,6 +133,13 @@ export interface UserConfig {
* Specify additional files to be treated as static assets.
*/
assetsInclude?: string | RegExp | (string | RegExp)[]
/**
* Specify additional extensions to be treated as sources of js modules
* requires plugins to be installed that transform the extensions!
*
* Plugin authors should set this with the config hook
*/
knownJsSrcExtensions?: string[]
/**
* Server specific options, e.g. host, port, https...
*/
Expand Down Expand Up @@ -195,7 +206,12 @@ export interface InlineConfig extends UserConfig {
export type ResolvedConfig = Readonly<
Omit<
UserConfig,
'plugins' | 'alias' | 'dedupe' | 'assetsInclude' | 'optimizeDeps'
| 'plugins'
| 'alias'
| 'dedupe'
| 'assetsInclude'
| 'optimizeDeps'
| 'knownJsSrcExtensions'
> & {
configFile: string | undefined
configFileDependencies: string[]
Expand All @@ -214,6 +230,7 @@ export type ResolvedConfig = Readonly<
server: ResolvedServerOptions
build: ResolvedBuildOptions
assetsInclude: (file: string) => boolean
knownJsSrcExtensions: string[]
logger: Logger
createResolver: (options?: Partial<InternalResolveOptions>) => ResolveFn
optimizeDeps: Omit<DepOptimizationOptions, 'keepNames'>
Expand Down Expand Up @@ -270,6 +287,11 @@ export async function resolveConfig(
// user config may provide an alternative mode
mode = config.mode || mode

// user config may provide alternative default known js src extensions
if (!config.knownJsSrcExtensions) {
config.knownJsSrcExtensions = [...DEFAULT_KNOWN_JS_SRC_EXTENSIONS]
}
Comment on lines +291 to +293
Copy link
Member

Choose a reason for hiding this comment

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

We can wait until we discuss this with Evan, but I think it is better if this works like assetsInclude. So the user will give us extra extensions and they are added to the ones supported by default by vite.


// resolve plugins
const rawUserPlugins = (config.plugins || []).flat().filter((p) => {
return p && (!p.apply || p.apply === command)
Expand Down Expand Up @@ -390,6 +412,15 @@ export async function resolveConfig(
)
: ''

//ensure leading dot and deduplicate
const resolvedKnownJsSrcExtensions = [
...new Set(
config.knownJsSrcExtensions?.map((ext) =>
ext.startsWith('.') ? ext : `.${ext}`
)
)
]

const resolved: ResolvedConfig = {
...config,
configFile: configFile ? normalizePath(configFile) : undefined,
Expand All @@ -416,6 +447,7 @@ export async function resolveConfig(
assetsInclude(file: string) {
return DEFAULT_ASSETS_RE.test(file) || assetsFilter(file)
},
knownJsSrcExtensions: resolvedKnownJsSrcExtensions,
Copy link
Member

Choose a reason for hiding this comment

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

We discussed a bit about this in Discord. IMO we could also use the same scheme utilized by assetsInclude here. In the ResolvedConfig, we will have a resolved.isJsRequest(path) function that plugins can then use. This is something else to discuss with Evan, if we go here, we may want to have all these helpers in resolve.utils.isJsRequest, but I am not sure if it is really needed and the flat structure is enough here. @antfu what do you think?

logger,
createResolver,
optimizeDeps: {
Expand Down
13 changes: 13 additions & 0 deletions packages/vite/src/node/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,19 @@ export const DEFAULT_EXTENSIONS = [
'.json'
]

// warning, these are only default values. At runtime you must check config.knownJsSrcExtensions
export const DEFAULT_KNOWN_JS_SRC_EXTENSIONS = [
'.js',
'.ts',
'.jsx',
'.tsx',
'.mjs',
// the extensions below should be removed once their respective plugins add them via config hook
'.vue',
'.marko',
'.svelte'
]

export const JS_TYPES_RE = /\.(?:j|t)sx?$|\.mjs$/

export const OPTIMIZABLE_ENTRY_RE = /\.(?:m?js|ts)$/
Expand Down
18 changes: 9 additions & 9 deletions packages/vite/src/node/plugins/importAnalysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ const clientDir = normalizePath(CLIENT_DIR)
const skipRE = /\.(map|json)$/
const canSkip = (id: string) => skipRE.test(id) || isDirectCSSRequest(id)

function isExplicitImportRequired(url: string) {
return !isJSRequest(cleanUrl(url)) && !isCSSRequest(url)
function isExplicitImportRequired(url: string, knownJSSrcExtensions: string[]) {
return !isJSRequest(url, knownJSSrcExtensions) && !isCSSRequest(url)
}

function markExplicitImport(url: string) {
if (isExplicitImportRequired(url)) {
function markExplicitImport(url: string, knownJSSrcExtensions: string[]) {
if (isExplicitImportRequired(url, knownJSSrcExtensions)) {
return injectQuery(url, 'import')
}
return url
Expand Down Expand Up @@ -89,7 +89,7 @@ function markExplicitImport(url: string) {
* ```
*/
export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
const { root, base } = config
const { root, base, knownJsSrcExtensions } = config
const clientPublicPath = path.posix.join(base, CLIENT_PUBLIC_PATH)

let server: ViteDevServer
Expand Down Expand Up @@ -120,7 +120,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
imports = parseImports(source)[0]
} catch (e) {
const isVue = importer.endsWith('.vue')
const maybeJSX = !isVue && isJSRequest(importer)
const maybeJSX = !isVue && isJSRequest(importer, knownJsSrcExtensions)

const msg = isVue
? `Install @vitejs/plugin-vue to handle .vue files.`
Expand Down Expand Up @@ -217,7 +217,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
// make the URL browser-valid if not SSR
if (!ssr) {
// mark non-js/css imports with `?import`
url = markExplicitImport(url)
url = markExplicitImport(url, knownJsSrcExtensions)

// for relative js/css imports, inherit importer's version query
// do not do this for unknown type imports, otherwise the appended
Expand Down Expand Up @@ -416,7 +416,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
}
if (
!/^('.*'|".*"|`.*`)$/.test(url) ||
isExplicitImportRequired(url.slice(1, -1))
isExplicitImportRequired(url.slice(1, -1), knownJsSrcExtensions)
) {
needQueryInjectHelper = true
str().overwrite(start, end, `__vite__injectQuery(${url}, 'import')`)
Expand Down Expand Up @@ -471,7 +471,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
const normalizedAcceptedUrls = new Set<string>()
for (const { url, start, end } of acceptedUrls) {
const [normalized] = await moduleGraph.resolveUrl(
toAbsoluteUrl(markExplicitImport(url))
toAbsoluteUrl(markExplicitImport(url, knownJsSrcExtensions))
)
normalizedAcceptedUrls.add(normalized)
str().overwrite(start, end, JSON.stringify(normalized))
Expand Down
4 changes: 2 additions & 2 deletions packages/vite/src/node/server/middlewares/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function transformMiddleware(
server: ViteDevServer
): Connect.NextHandleFunction {
const {
config: { root, logger, cacheDir },
config: { root, logger, cacheDir, knownJsSrcExtensions },
moduleGraph
} = server

Expand Down Expand Up @@ -132,7 +132,7 @@ export function transformMiddleware(
}

if (
isJSRequest(url) ||
isJSRequest(url, knownJsSrcExtensions) ||
isImportRequest(url) ||
isCSSRequest(url) ||
isHTMLProxy(url)
Expand Down
15 changes: 6 additions & 9 deletions packages/vite/src/node/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,16 +106,13 @@ export const isExternalUrl = (url: string): boolean => externalRE.test(url)
export const dataUrlRE = /^\s*data:/i
export const isDataUrl = (url: string): boolean => dataUrlRE.test(url)

const knownJsSrcRE = /\.((j|t)sx?|mjs|vue|marko|svelte)($|\?)/
export const isJSRequest = (url: string): boolean => {
export const isJSRequest = (
url: string,
knownJSSrcExtensions: string[]
): boolean => {
url = cleanUrl(url)
if (knownJsSrcRE.test(url)) {
return true
}
if (!path.extname(url) && !url.endsWith('/')) {
return true
}
return false
const ext = path.extname(url)
return ext ? knownJSSrcExtensions.includes(ext) : !url.endsWith('/')
}

const importQueryRE = /(\?|&)import=?(?:&|$)/
Expand Down