Skip to content

fix: module rewrite in unoptimized dep#344

Merged
yyx990803 merged 9 commits into
vitejs:masterfrom
csr632:fix/rewrite-unoptimized
Jun 4, 2020
Merged

fix: module rewrite in unoptimized dep#344
yyx990803 merged 9 commits into
vitejs:masterfrom
csr632:fix/rewrite-unoptimized

Conversation

@csr632
Copy link
Copy Markdown
Member

@csr632 csr632 commented Jun 3, 2020

This is a regression of #228

You can see the reproduction in the playground.

Comment thread src/node/resolver.ts
* 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
Copy Markdown
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

let { pathname, query } = resolveRelativeRequest(importer, id)

// 2. resolve extensions.
const ext = resolver.resolveExt(pathname)
Copy link
Copy Markdown
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

@csr632 csr632 force-pushed the fix/rewrite-unoptimized branch from 60cf615 to 9bc5c2b Compare June 4, 2020 02:34
Comment thread src/node/resolver.ts
const ext = resolveExt(resolved)
const result = {
filePath: ext ? resolved + ext : resolved,
ext: ext || path.extname(resolved)
Copy link
Copy Markdown
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.

@csr632
Copy link
Copy Markdown
Member Author

csr632 commented Jun 4, 2020

I don't know why test is failling, it seems like jest issue with esModule?

@yyx990803
Copy link
Copy Markdown
Member

The implementation seems to have a lot of problems in Windows due to not distinguishing between request paths vs. file paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants