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(optimizer): check optimizeDeps.extensions for scannable files #14543

Merged

Conversation

DylanPiercey
Copy link
Contributor

Description

Updates the isScannable logic to include extensions registered via optimizeDeps.extensions.
This allows plugins (such as @marko/vite) to contribute to the optimized dependency scanning if they supply an esbuild plugin.

Additional context

Currently due to the way @marko/vite is setup it relied exclusively on optimizeDeps.include and not the dependency scanning so adding this change does break existing Marko apps.

However @marko/vite is being updated to use optimizeDeps.entries instead since the include method we were using needlessly bundles more than necessary.

The actual processing of Marko files is done by an esbuild hook added by @marko/vite so no further change is needed here other than allowing .marko files to pass through the isScannable checks.

This issue was addressed in a different way in #14536 and so if this solution is deemed better we can close that PR.


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 PR Title 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.

@DylanPiercey DylanPiercey changed the title fix: include optimizeDeps.extensions when checking if a file can be s… fix: include optimizeDeps.extensions when checking if a file can be scanned Oct 5, 2023
@DylanPiercey
Copy link
Contributor Author

Actually hold on a second my logic isn't correct in the isScannable implementation.

@DylanPiercey DylanPiercey force-pushed the scan-include-optimize-extensions branch from 089b06e to f571795 Compare October 5, 2023 16:01
@DylanPiercey
Copy link
Contributor Author

@bluwy it should be good now. Let me know if you have any feedback on this implementation!

@bluwy bluwy added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Oct 5, 2023
@DylanPiercey DylanPiercey force-pushed the scan-include-optimize-extensions branch from f571795 to bec9eb2 Compare October 5, 2023 16:40
@DylanPiercey
Copy link
Contributor Author

@bluwy I went ahead and reverted to my original implementation (with path.extname).

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.

Awesome. Thanks! Will wait for one more set of eyes for this since it's expanding an existing feature.

@bluwy bluwy changed the title fix: include optimizeDeps.extensions when checking if a file can be scanned feat(optimizer): check optimizeDeps.extensions for scannable files Oct 5, 2023
@DylanPiercey
Copy link
Contributor Author

@bluwy is there a timeline for the Vite 5 release? If it's expected to be a while it would be nice for us if this could be backported. 🙏

@bluwy
Copy link
Member

bluwy commented Oct 6, 2023

The Vite 5 stable release is planned mid October, though with the progress so far it could be a little late, but shouldn't extend till next month.

I don't think this fix could be backported though, it's leaning towards a bigger change that could affect downstream (actually let me run this through ecosystem-ci). If we would to backport something, I think it would be #14536 instead.

@bluwy
Copy link
Member

bluwy commented Oct 6, 2023

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Oct 6, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ✅ success
histoire ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ❌ failure
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vike ✅ success
vite-plugin-pwa ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

This makes sense to me, and it kind of feels more like a fix than a new feature. I think we can merge it.

@patak-dev patak-dev merged commit 23ef8a1 into vitejs:main Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants