fix(config): don't use `module` condition (fixes #10430) by sapphi-red · Pull Request #10495 · vitejs/vite

sapphi-red · 2022-10-17T08:11:47Z

Description

node does not use module condition. But Vite was using that when bundling (replacing specifier with absolute path) config.
This PR uses esbuild to resolve specifier instead of Vite's internal resolver.

I came up with three solutions for this.

Add noModuleCondition option to vite's internal resolver and use that for config bundling (similar to fix(config): don't resolve by exports.module field #10442)
- In Vite 4, I think we could clean up these options but I think it's better to avoid introducing a new option for now.
Use esbuild to resolve specifiers
- As we don't need fine-grained option and just need to imitate node's resolver, I think using esbuild's one is more performant. Also we could avoid a new option. This PR is implementing in this way.
Use import.meta.resolve / import-meta-resolve (ponyfill)
- import-meta-resolve is only 63.3kB and adding this won't affect the package size much. We could perfectly imitate node's resolver by using this. But I think we could just use esbuild's resolver as it's reliable enough and could be more performant.

superseds #10442
fixes #10430
refs #10254 #10347 #10420

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the Commit Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

bluwy

I've also been thinking of a few ways to fix this, and this is much much elegant!

bluwy · 2022-10-17T13:16:55Z

packages/vite/src/node/config.ts

+              if (result.external) {
+                return { path: result.path, external: true }
              }
+              const absolutePath = path.resolve(importer, '..', result.path)


Suggested change

const absolutePath = path.resolve(importer, '..', result.path)

const absolutePath = path.resolve(path.dirname(importer), result.path)

I'm not sure if it makes a difference, but I usually use path.dirname. But the .. trick here seems to work too.

patak-cat · 2022-10-17T15:49:04Z

/ecosystem-ci run

vite-ecosystem-ci · 2022-10-17T15:49:26Z

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	✅ success
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	✅ success
rakkas	✅ success
svelte	❌ failure
vite-plugin-ssr	❌ failure
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	❌ failure
windicss	✅ success

patak-cat · 2022-10-17T16:06:54Z

@sapphi-red vite-plugin-ssr and SvelteKit are also failing in main, but the Vitest fail seems to be related to this PR 🤔

benmccann · 2022-10-17T16:48:54Z

Could we add a test to ensure we don't revert #10430 in the future?

sapphi-red · 2022-10-18T08:58:01Z

It seems Vitest is failing because esbuild uses paths in tsconfig.json. I didn't find a way to disable this. Maybe we have to do it in a different way.

Sure. I'll add tests when we decided the approach. 👍

@bluwy What way did you have in mind?

bluwy · 2022-10-18T14:32:51Z

You might hate me for this, but tsconfig: 'package.json' works for Vitest for me 🙈 We could point to Vite's package.json instead to disable tsconfig altogether since I don't think it's relevant for nodejs bundling (?).

I really like the esbuild solution, but otherwise we may have to use import-meta-resolve.

sapphi-red · 2022-10-19T06:54:22Z

That's neat. But unfortunately I guess that will break scripts using useDefineForClassFields.

bluwy · 2022-10-19T07:22:44Z

Hmm yeah maybe an alternative for now is to go with no1. The initial idea I had was to introduce a strictNodeResolve: boolean option, so that allows us to expand more in the future. We can start with removing the module condition, and possibly the production/development condition?

If it happens to be a lot of work, import-meta-resolve would be the easier way out.

sapphi-red · 2022-10-20T07:15:47Z

superseded by #10528

fix(config): don't use module condition

082fe4d

sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 17, 2022

sapphi-red changed the title ~~fix(config): don't use module condition~~ fix(config): don't use module condition (fixes #10430) Oct 17, 2022

sapphi-red added p4-important Violate documented behavior or significantly improves performance (priority) and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Oct 17, 2022

bluwy approved these changes Oct 17, 2022

View reviewed changes

benmccann mentioned this pull request Oct 18, 2022

docs: add docs for module condition #10524

Closed

9 tasks

sapphi-red mentioned this pull request Oct 19, 2022

fix(config): don't use module condition (import.meta.resolve) (fixes #10430) #10528

Merged

9 tasks

sapphi-red closed this Oct 20, 2022

sapphi-red deleted the fix/config-node-like-resolve branch October 20, 2022 07:16

sapphi-red mentioned this pull request Oct 28, 2022

fix(config): resolve externalized specifier with internal resolver #10683

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(config): don't use `module` condition (fixes #10430)#10495

fix(config): don't use `module` condition (fixes #10430)#10495
sapphi-red wants to merge 1 commit intovitejs:mainfrom
sapphi-red:fix/config-node-like-resolve

sapphi-red commented Oct 17, 2022 •

edited

Loading

Uh oh!

bluwy left a comment

Uh oh!

bluwy Oct 17, 2022

Uh oh!

patak-cat commented Oct 17, 2022

Uh oh!

vite-ecosystem-ci bot commented Oct 17, 2022 •

edited

Loading

Uh oh!

patak-cat commented Oct 17, 2022

Uh oh!

benmccann commented Oct 17, 2022

Uh oh!

sapphi-red commented Oct 18, 2022

Uh oh!

bluwy commented Oct 18, 2022

Uh oh!

sapphi-red commented Oct 19, 2022

Uh oh!

bluwy commented Oct 19, 2022

Uh oh!

sapphi-red commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	const absolutePath = path.resolve(importer, '..', result.path)
	const absolutePath = path.resolve(path.dirname(importer), result.path)

Uh oh!

Conversation

sapphi-red commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

bluwy Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

patak-cat commented Oct 17, 2022

Uh oh!

vite-ecosystem-ci bot commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patak-cat commented Oct 17, 2022

Uh oh!

benmccann commented Oct 17, 2022

Uh oh!

sapphi-red commented Oct 18, 2022

Uh oh!

bluwy commented Oct 18, 2022

Uh oh!

sapphi-red commented Oct 19, 2022

Uh oh!

bluwy commented Oct 19, 2022

Uh oh!

sapphi-red commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sapphi-red commented Oct 17, 2022 •

edited

Loading

vite-ecosystem-ci bot commented Oct 17, 2022 •

edited

Loading