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(config): don't use module condition (fixes #10430) #10495

Closed

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Oct 17, 2022

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.

  1. 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.
  2. 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.
  3. 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.

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 17, 2022
@sapphi-red 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 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
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

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

}
const absolutePath = path.resolve(importer, '..', result.path)
Copy link
Member

Choose a reason for hiding this comment

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

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-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Oct 17, 2022

📝 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-dev
Copy link
Member

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

@benmccann
Copy link
Collaborator

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

@sapphi-red
Copy link
Member Author

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
Copy link
Member

bluwy commented Oct 18, 2022

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
Copy link
Member Author

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

@bluwy
Copy link
Member

bluwy commented Oct 19, 2022

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
Copy link
Member Author

superseded by #10528

@sapphi-red sapphi-red closed this Oct 20, 2022
@sapphi-red sapphi-red deleted the fix/config-node-like-resolve branch October 20, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite >=3.1.5 breaks Jotai babel plugins
4 participants