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

fix: module rewrite in unoptimized dep #344

Merged
merged 9 commits into from
Jun 4, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
5 changes: 4 additions & 1 deletion playground/App.vue
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
<TestRewriteOptimized />
<TestCustomBlocks />
<TestOptimizeLink />
<TestRewriteUnoptimized />
</template>

<script>
Expand All @@ -48,6 +49,7 @@ import TestRewriteOptimized from './rewrite-optimized/TestRewriteOptimized.vue'
import TestCssAtImport from './css-@import/TestCssAtImport.vue'
import TestCustomBlocks from './custom-blocks/TestCustomBlocks.vue'
import TestOptimizeLink from './optimize-linked/TestOptimizeLink.vue'
import TestRewriteUnoptimized from './rewrite-unoptimized/TestRewriteUnoptimized.vue'

export default {
data: () => ({
Expand All @@ -73,7 +75,8 @@ export default {
TestAsync: defineAsyncComponent(() => import('./TestAsync.vue')),
TestRewriteOptimized,
TestCustomBlocks,
TestOptimizeLink
TestOptimizeLink,
TestRewriteUnoptimized
}
}
</script>
3 changes: 2 additions & 1 deletion playground/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"normalize.css": "link:../node_modules/normalize.css",
"resolve-browser-field-test-package": "link:./resolve-browser-field",
"rewrite-optimized-test-package": "link:./rewrite-optimized/test-package",
"optimize-linked": "link:./optimize-linked"
"optimize-linked": "link:./optimize-linked",
"rewrite-unoptimized-test-package": "link:./rewrite-unoptimized/test-package"
}
}
12 changes: 12 additions & 0 deletions playground/rewrite-unoptimized/TestRewriteUnoptimized.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<template>
<h2>Test rewrite in optimized module</h2>
<p class="test-rewrite-in-unoptimized">{{ msg }}</p>
</template>

<script>
import value from 'rewrite-unoptimized-test-package'

export default {
data: () => ({ msg: value })
}
</script>
3 changes: 3 additions & 0 deletions playground/rewrite-unoptimized/test-package/es/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { Item } from './nested'

export default Item
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as Item } from './test'
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 123
6 changes: 6 additions & 0 deletions playground/rewrite-unoptimized/test-package/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "rewrite-unoptimized-test-package",
"version": "1.0.0",
"main": "es/index.js",
"license": "MIT"
}
2 changes: 1 addition & 1 deletion playground/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const config: UserConfig = {
plugins: [jsPlugin],
vueCustomBlockTransforms: { i18n: i18nTransform },
optimizeDeps: {
exclude: ['bootstrap'],
exclude: ['bootstrap', 'rewrite-unoptimized-test-package'],
link: ['optimize-linked']
}
}
Expand Down
8 changes: 8 additions & 0 deletions playground/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
version "0.0.0"
uid ""

"linked-dep@link:./optimize-linked/linked-dep":
version "0.0.0"
uid ""

"lodash-es@link:../node_modules/lodash-es":
version "0.0.0"
uid ""
Expand All @@ -29,3 +33,7 @@
"rewrite-optimized-test-package@link:./rewrite-optimized/test-package":
version "0.0.0"
uid ""

"rewrite-unoptimized-test-package@link:./rewrite-unoptimized/test-package":
version "0.0.0"
uid ""
138 changes: 99 additions & 39 deletions src/node/resolver.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import fs from 'fs-extra'
import path from 'path'
import slash from 'slash'
import { cleanUrl, resolveFrom, queryRE } from './utils'
import {
cleanUrl,
resolveFrom,
queryRE,
lookupFile,
parseNodeModuleId
} from './utils'
import {
moduleRE,
moduleIdToFileMap,
Expand All @@ -21,8 +27,8 @@ export interface Resolver {
export interface InternalResolver {
requestToFile(publicPath: string): string
fileToRequest(filePath: string): string
normalizePublicPath(publicPath: string): string
alias(id: string): string | undefined
resolveExt(publicPath: string): string | undefined
}

export const supportedExts = ['.mjs', '.js', '.ts', '.jsx', '.tsx', '.json']
Expand Down Expand Up @@ -70,26 +76,34 @@ const isFile = (file: string): boolean => {
}
}

const resolveExt = (id: string): string | undefined => {
const cleanId = cleanUrl(id)
if (!isFile(cleanId)) {
let inferredExt = ''
/**
* this function resolve fuzzy file path. examples:
* /path/file is a fuzzy file path for /path/file.tsx
* /path/dir is a fuzzy file path for /path/dir/index.js
*
* returning undefined indicates the filePath is not fuzzy:
* it is already an exact file path, or it can't match any file
*/
const resolveFilePathPostfix = (filePath: string): string | undefined => {
Copy link
Member Author

@csr632 csr632 Jun 3, 2020

Choose a reason for hiding this comment

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

In order to avoid misuse again in the future, I rename this function and add comment

const cleanPath = cleanUrl(filePath)
if (!isFile(cleanPath)) {
let postfix = ''
for (const ext of supportedExts) {
if (isFile(cleanId + ext)) {
inferredExt = ext
if (isFile(cleanPath + ext)) {
postfix = ext
break
}
if (isFile(path.join(cleanId, '/index' + ext))) {
inferredExt = '/index' + ext
if (isFile(path.join(cleanPath, '/index' + ext))) {
postfix = '/index' + ext
break
}
}
const queryMatch = id.match(/\?.*$/)
const queryMatch = filePath.match(/\?.*$/)
const query = queryMatch ? queryMatch[0] : ''
const resolved = cleanId + inferredExt + query
if (resolved !== id) {
debug(`(extension) ${id} -> ${resolved}`)
return inferredExt
const resolved = cleanPath + postfix + query
if (resolved !== filePath) {
debug(`(postfix) ${filePath} -> ${resolved}`)
return postfix
}
}
}
Expand Down Expand Up @@ -141,17 +155,12 @@ export function createResolver(
})
resolveAlias(userAlias)

const requestToFileCache = new Map()
const fileToRequestCache = new Map()
const requestToFileCache = new Map<string, string>()
const fileToRequestCache = new Map<string, string>()

const resolveRequest = (
publicPath: string
): {
filePath: string
ext: string | undefined
} => {
const resolveRequest = (publicPath: string): string => {
if (requestToFileCache.has(publicPath)) {
return requestToFileCache.get(publicPath)
return requestToFileCache.get(publicPath)!
}

let resolved: string | undefined
Expand All @@ -165,27 +174,21 @@ export function createResolver(
if (!resolved) {
resolved = defaultRequestToFile(publicPath, root)
}
const ext = resolveExt(resolved)
const result = {
filePath: ext ? resolved + ext : resolved,
ext: ext || path.extname(resolved)
Copy link
Member Author

@csr632 csr632 Jun 4, 2020

Choose a reason for hiding this comment

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

Previous code can't guarantee ext is the actual missing postfix of publicPath, because defaultRequestToFile would add /index.js to resolved silently, and ext only return .js here.

}
const postfix = resolveFilePathPostfix(resolved)
// TODO resolved may contain query
const result = postfix ? resolved + postfix : resolved
requestToFileCache.set(publicPath, result)
return result
}

return {
requestToFile(publicPath) {
return resolveRequest(publicPath).filePath
},

resolveExt(publicPath) {
return resolveRequest(publicPath).ext
return resolveRequest(publicPath)
},

fileToRequest(filePath) {
if (fileToRequestCache.has(filePath)) {
return fileToRequestCache.get(filePath)
return fileToRequestCache.get(filePath)!
}
for (const r of resolvers) {
const request = r.fileToRequest && r.fileToRequest(filePath, root)
Expand All @@ -196,6 +199,48 @@ export function createResolver(
return res
},

normalizePublicPath(publicPath) {
// preserve query
const queryMatch = publicPath.match(/\?.*$/)
const query = queryMatch ? queryMatch[0] : ''
const cleanPublicPath = cleanUrl(publicPath)
const result = main(cleanPublicPath, this) + query
// double check
if (this.requestToFile(result) !== this.requestToFile(publicPath)) {
throw new Error(
`[vite] normalizePublicPath check fail. please report to vite.`
)
}
return result

function main(publicPath: string, resolver: InternalResolver) {
if (!moduleRE.test(publicPath))
return resolver.fileToRequest(resolver.requestToFile(publicPath))

const filePath = resolver.requestToFile(publicPath)
const optimizedPublicPath = recognizeOptimizedFilePath(root, filePath)
if (optimizedPublicPath) return optimizedPublicPath

// fileToRequest doesn't work with files in node_modules
// because of edge cases like symlinks or yarn-aliased-install
// or even aliased-symlinks

// /@modules/@scope/test-package -> /@modules/@scope/test-package
const id = publicPath.replace(moduleRE, '')
const { scope, name, inPkgPath } = parseNodeModuleId(id)
if (!inPkgPath) return publicPath
// /@modules/test-package/dir -> /@modules/test-package/dir/index.js
const pkgPath = lookupFile(filePath, ['package.json'], true)
if (!pkgPath)
throw new Error(
`[vite] can't find package.json for a node_module file: ` +
`"${publicPath}". something is wrong.`
)
const inLibPath = path.relative(path.dirname(pkgPath), filePath)
return ['/@modules', scope, name, inLibPath].filter(Boolean).join('/')
}
},

alias(id) {
let aliased: string | undefined = literalAlias[id]
if (aliased) {
Expand Down Expand Up @@ -305,6 +350,21 @@ export function resolveOptimizedModule(
}
}

/**
* if filePath is in the optimized cache dir,
* return the public path for it
*/
function recognizeOptimizedFilePath(
root: string,
filePath: string
): string | undefined {
const cacheDir = resolveOptimizedCacheDir(root)
if (!cacheDir) return
const relative = path.relative(cacheDir, filePath)
if (!relative.startsWith('../'))
return relative.replace(/^\.\.\//, '/@modules/')
}

interface NodeModuleInfo {
entry: string | null
entryFilePath: string | null
Expand Down Expand Up @@ -371,10 +431,10 @@ export function resolveNodeModule(
if (entryPoint) {
// #284 some packages specify entry without extension...
entryFilePath = path.join(path.dirname(pkgPath), entryPoint!)
const ext = resolveExt(entryFilePath)
if (ext) {
entryPoint += ext
entryFilePath += ext
const postfix = resolveFilePathPostfix(entryFilePath)
if (postfix) {
entryPoint += postfix
entryFilePath += postfix
}
entryPoint = path.posix.join(id, entryPoint!)
// save the resolved file path now so we don't need to do it again in
Expand Down
11 changes: 3 additions & 8 deletions src/node/server/serverPluginModuleRewrite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,7 @@ export const moduleRewritePlugin: ServerPlugin = ({
// before we perform hmr analysis.
// on the other hand, static import is guaranteed to have extension
// because they must all have gone through module rewrite.
const importer = resolver.fileToRequest(
resolver.requestToFile(ctx.path)
)
const importer = resolver.normalizePublicPath(ctx.path)
ctx.body = rewriteImports(
root,
content!,
Expand Down Expand Up @@ -254,11 +252,8 @@ export const resolveImport = (
// ./foo -> /some/path/foo
let { pathname, query } = resolveRelativeRequest(importer, id)

// 2. resolve extensions.
const ext = resolver.resolveExt(pathname)
Copy link
Member Author

@csr632 csr632 Jun 3, 2020

Choose a reason for hiding this comment

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

This is the cause of bug. resolveExt is misused.
For pathname /test-package/lib/dir, resolveExt returned .js instead of /index.js here.
So it is rewritten into /test-package/lib/dir.js instead of /test-package/lib/dir/index.js

if (ext && ext !== path.extname(pathname)) {
pathname = path.posix.normalize(pathname + ext)
}
// 2. resolve dir index and extensions.
pathname = resolver.normalizePublicPath(pathname)

// 3. mark non-src imports
if (!query && path.extname(pathname) && !jsSrcRE.test(pathname)) {
Expand Down
15 changes: 15 additions & 0 deletions src/node/utils/pathUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,18 @@ export const isStaticAsset = (file: string) => {
export const isImportRequest = (ctx: Context): boolean => {
return ctx.query.import != null
}

export function parseNodeModuleId(id: string) {
const parts = id.split('/')
let scope = '',
name = '',
inPkgPath = ''
if (id.startsWith('@')) scope = parts.shift()!
name = parts.shift()!
inPkgPath = parts.join('/')
return {
scope,
name,
inPkgPath
}
}
4 changes: 4 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,10 @@ describe('vite', () => {
)
})

test('rewrite import in unoptimized deps', async () => {
expect(await getText('.test-rewrite-in-unoptimized')).toMatch('123')
})

test('monorepo support', async () => {
// linked dep + optimizing linked dep
expect(await getText(`.optimize-linked`)).toMatch(`ok`)
Expand Down