ciscoheat / sveltekit-superforms

Making SvelteKit forms a pleasure to use!
https://superforms.rocks
MIT License
2.13k stars 62 forks source link

All validation libraries from optional dependencies are installed in dev environment #351

Open vladshcherbin opened 6 months ago

vladshcherbin commented 6 months ago

Description Found in https://github.com/ciscoheat/sveltekit-superforms/issues/350

Currently all validation libraries are installed even when they are not used (all optional dependencies). This is not good for a lot of reasons - wasted space, traffic, package stats update, etc.

From referenced issue it's also visible that some bundlers (vite in my case) are trying to process/include this dependencies in the bundle which is also not desired.

You can see a massive library size increase: https://packagephobia.com/result?p=sveltekit-superforms@2.3.0

image

I've tried different combinations of commands with no luck to install only what I need:

yarn add sveltekit-superforms
yarn add sveltekit-superforms valibot
yarn add sveltekit-superforms -D
yarn add sveltekit-superforms valibot -D

A solution should be found on how to get only used packages 🤔

ciscoheat commented 6 months ago

This is not accurate, the packages are only installed in a dev environment. Vite/Rollup will only bundle what's referenced in the project. For example, when I build with Valibot:

2024-02-19 13_19_07-+page svelte - sf2-bundle - Visual Studio Code
vladshcherbin commented 6 months ago

I've added a reproduction with screenshot - https://github.com/vladshcherbin/svelte-superforms.

From screenshot it's visible vite is at least processing zod-to-json-schema which is not used anywhere in the app.

From superforms & valibot dependency commit it's visible how many unused packages are installed which is also not desirable for the reasons above.

I'd be happy to help solve but have no idea except splitting adapters into different packages and install only needed e.g.

yarn add sveltekit-superforms sveltekit-superforms-adapters/valibot valibot
ciscoheat commented 6 months ago

What's installed during development is not an issue, I'm concerned with the final bundle size. You can even look through the non-minified output to see that Zod is never referenced anywhere.

ciscoheat commented 6 months ago

About the circular dependency, it doesn't happen with pnpm, so maybe it's a yarn issue.

vladshcherbin commented 6 months ago

Well, that's the best reproduction I can give. I do believe both issues should be solved and only required libraries be installed but I respect your opinion, direction and ideas.

Sorry for opening so many issues and thanks for the library, everything else seems to be working and simplifying forms is a huge relief 😇

Hopefully this ones can be resolved some day 🙌

gregmsanderson commented 6 months ago

If it helps at all, I came across this issue while investigating this warning:

vite v5.1.3 building for production... [plugin:vite:resolve] Module "node:dns/promises" has been externalized for browser compatibility, imported by "/node_modules/@vinejs/vine/build/chunk-TVEIURSI.js". See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

I've been playing with superforms (via formsnap). It's great! But I was puzzled where @vinejs was coming from, having not installed it. From: npm ls --all I noticed a peer dependency.

I agree that (in my case) @vinejs won't be in the production bundle ... however its presence does seemingly result in that Vite warning while bundling it. Vine appears to be form validation focused on Node apps and so while they might change it at their end, not including it as a peer dependency would also avoid that warning. I would assume people would install their choice of validation library separately.

I do agree it's an enhancement since a warning isn't an error.

fnimick commented 2 weeks ago

FYI, this now causes issues with usage of arktype since the optional version of arktype (2.0.0-beta.0) has breaking changes to the current version (2.0.0-rc.0) and installing the old version results in it being preferred for imports and causing type errors under certain circumstances.

https://github.com/sveltejs/language-tools/issues/2474

fnimick commented 2 weeks ago

I was able to fix the arktype version conflicts by adding the following to my package.json:

  "overrides": {
    "sveltekit-superforms": {
      "arktype": "$arktype"
    }
  }

(I use npm 10.x, the override format may be different for other package managers)

ciscoheat commented 2 weeks ago

Thank you for looking into this, @fnimick! Do you know if this is the required fix, or if Superforms or Arktype can do anything to handle it without user modification of package.json?

fnimick commented 2 weeks ago

You can have superforms specify the -rc.0 version as an optional dependency, but that will then run into the issue where people currently using the beta builds with the incompatible API will have the same problem but in reverse, no?

Maybe the solution is to specify the optional dependency using a semver tag, like you do for peer dependencies? That way people will install superforms + arktype (presumably) at the same time and get compatible versions, but won't get the transitive dependency automatically updated unless they specifically do so? No overrides required.

(and specifying optional dependencies using version ranges rather than fixed versions I would recommend for all of the dependencies, in that case, not just arktype)

fnimick commented 2 weeks ago

Another option is to remove optional dependencies entirely and rely on peerDependencies for validation library installation / management. Though then you'd have to figure out how to handle the case where the user attempts to use / import an adapter for a library they don't have installed.

fnimick commented 2 weeks ago

fyi @ciscoheat someone from the svelte team did an investigation and found that there's no workaround short of avoiding the version mismatch entirely, using one of the strategies outlined above. https://github.com/sveltejs/language-tools/issues/2474#issuecomment-2314609158