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

Improve handling for hybrid svelte packages in automatic dependency optimization #162

Closed
dominikg opened this issue Sep 4, 2021 · 11 comments · Fixed by #163
Closed

Improve handling for hybrid svelte packages in automatic dependency optimization #162

dominikg opened this issue Sep 4, 2021 · 11 comments · Fixed by #163
Labels
enhancement New feature or request

Comments

@dominikg
Copy link
Member

dominikg commented Sep 4, 2021

Describe the problem

Some svelte packages are hybrids and could contain dependencies on node-only packages.

These packages then can fail in vite's optimizer with an error like
Failed to resolve entry for package "foo > bar". The package may have incorrect main/module/exports specified in its package.json

Currently known hybrids:

  • @roxi/routify - error caused by @types/node (only types entry), cheap-watch, rollup-pluginutils (export esm and used node builtin modules)
  • @carbon/charts-svelte - error caused by @carbon/telemetry (only bin entry)

Describe the proposed solution

  • detect dependencies that would fail in vite's optimizer and don't reinclude them.
  • detect dependencies that are not used in svelte components and don't reinclude them

Alternatives considered

  • Improve vite's optimizer to allow deep scanning
  • Hardcode list of known hybrids
  • Keep the status quo where people have to opt-out of the automatic behavior
    • either by adding the dependencies that fail in vite's optimizer to optimizeDeps.exclude in vite config
    • or by adding the declaring package to vite-plugin-svelte's disableDependencyReinclusion option.
  • Change automatic dependency handling to be opt-in in vite-plugin-svelte

Importance

would make my life easier

@dominikg dominikg added enhancement New feature or request triage Awaiting triage by a project member and removed triage Awaiting triage by a project member labels Sep 4, 2021
@dominikg
Copy link
Member Author

dominikg commented Sep 4, 2021

Issues in other repos:

carbon-design-system/carbon-charts#1130
vitejs/vite#4837

@benmccann
Copy link
Member

Another alternative: We change nothing in vite-plugin-svelte, let the packages be optimized, and fix whatever's causing the failure. Looking at the bug report for carbon-charts it seems they either have a misconfigured package.json or possibly Vite has a bug in package resolving. We could try to fix whatever's causing the failure and not touch this package.

@bluwy
Copy link
Member

bluwy commented Sep 4, 2021

@benmccann I've updated the description with some info on why the packages fail.

For @types/node and @carbon/telemetry, their only entrypoints are types and bin, and that makes sense as they're only ever used in those contexts (type definition and command). The best heuristic we could use for these packages are by looking up their package.json and checks if they have main/module/export (other custom main fields won't be checked though but that's likely a packaging issue within the library itself.

For cheap-watch and rollup-pluginutil, this is really weird. These are node-only deps, but Routify also has other node-only deps, so why these two libraries only specifically? Turns out these two libraries export ESM code, which tripped Vite's pre-bundling to this error:

node_modules/cheap-watch/dist/CheapWatch.esm.js:2:9: error: No matching export in "browser-external:fs" for import "watch"

The other node-only deps only exported CJS code and Vite worked fine with them (Likely because the prebundling needsInterop metadata helped caught these?). But this whole kerfuffle is ironic in general since we would always encourage libraries to export ESM code.


One solution I had in mind was to implement our own scanner and detect used dependencies ourselves, instead of the current implementation that puts all the library's dependencies into optimization. It could be tricky though with a lot of unknowns ahead.

Otherwise, ideally we could fix by letting Vite do all the hard work of nested optimization by default, addressing Improve vite's optimizer to allow deep scanning, but I haven't looked into how feasible that is yet.

@ghostdevv
Copy link
Member

In Routify, packages such as cheap-watch are only used in the cli that is baked in to the @roxi/routify package. Is the issue that vite is picking those up and assuming they are being shipped to the frontend so processing them?

@bluwy
Copy link
Member

bluwy commented Sep 4, 2021

Is the issue that vite is picking those up and assuming they are being shipped to the frontend so processing them?

Yeah, but it's vite-plugin-svelte instead of vite making the assumption since currently it thinks that all dependencies of a Svelte lib will be used in the frontend.

@dominikg
Copy link
Member Author

dominikg commented Sep 4, 2021

yes, but it is vite-plugin-svelte making this assumption, not vite itself.
But ultimately Ben is right, if Vite's optimizer was able to work things out correctly itself we would not be playing patchup here

@benmccann
Copy link
Member

Someone had tried using cheap-watch with ESM outside of Vite and reported it didn't work eventhough it provides an ESM version. I'm not sure if it'd help here, but they sent a PR to fix: Conduitry/cheap-watch#10. Perhaps we could try with that fix

@ghostdevv
Copy link
Member

Is there an issue on vite for them assuming all deps are to be optimised?

@bluwy
Copy link
Member

bluwy commented Sep 5, 2021

Is there an issue on vite for them assuming all deps are to be optimised?

I don't think so, pre-bundling/optimization is only for client-side deps only. Currently, vite-plugin-svelte is forcing vite to optimize node-only deps so that trips it up. I'm not sure if that's something they want to fix.


However, I may have a solution that should fix all these issues directly in vite-plugin-svelte: https://github.com/sveltejs/vite-plugin-svelte/compare/fix/safer-nested-optimization. It's pretty rough but it should resolve the two issues I mentioned in my comment.

@ghostdevv
Copy link
Member

However, I may have a solution that should fix all these issues directly in vite-plugin-svelte

Would this only resolve svelte dependancies? what happens to deps without a svelte field in their package.json

@dominikg
Copy link
Member Author

dominikg commented Sep 5, 2021

The whole automatic dependency optimization handling is only done for dependencies with "svelte" in their package.json. Regular non-svelte dependencies are handled by vite itself.

Svelte dependencies are special because we have to exclude them from optimization to avoid errors. And then reinclude their cjs dependencies to avoid subsequent errors 🙈 .

Ideally vite's optimizer would be able to understand svelte dependencies correctly (with the help of vite-plugin-svelte or internal fixes in vite), but until we get there, we can only try to make it work as good as possible.

The fix proposed by @bluwy prevents reincluding dependencies that would cause the errors mentioned above, which is good because less users would be affected. But it's not the long term solution we're after. And it requires additional parsing of package.json files, so it affects startup time of vite-plugin-svelte, esp. in projects with many dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants