ciscoheat / sveltekit-superforms

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

Circular dependency warning when using @sveltejs/adapter-node #350

Open vladshcherbin opened 9 months ago

vladshcherbin commented 9 months ago

Description When using SvelteKit, vite build gives Circular dependency warning:

image

The build finishes and is working though. Would be great to find a way to get rid of it 🙌

sveltekit-superforms - 2.3.0 @sveltejs/adapter-node - 4.0.1 @sveltejs/kit - 2.5.0 svelte - 4.2.11

ciscoheat commented 9 months ago

Try to reproduce it without Superforms, as it seems related to zod-to-json-schema.

vladshcherbin commented 9 months ago

@ciscoheat yeah, I definitely think it's related only to zod-to-json-schema and I think most of users (me included) use it only in superforms and not directly.

That's why I thought of a maybe possible solution of its imports update in superforms (directly to files of it for example) to overcome the Circular dependency warning.

vladshcherbin commented 9 months ago

I'm also wondering why I even have this warning, I don't use zod with superforms or anywhere in the app 🤔

vladshcherbin commented 9 months ago

@ciscoheat I've checked and when superforms package is added with:

yarn add sveltekit-superforms

seems like all optional packages are installed:

success Saved lockfile.
success Saved 29 new dependencies.
info Direct dependencies
└─ sveltekit-superforms@2.3.0
info All dependencies
├─ @gcornut/valibot-json-schema@0.0.22
├─ @hapi/topo@5.1.0
├─ @poppinss/macroable@1.0.1
├─ @sideway/address@4.1.5
├─ @sideway/formula@3.0.1
├─ @sideway/pinpoint@2.0.0
├─ @sinclair/typebox@0.32.14
├─ @sodaru/yup-to-json-schema@2.0.1
├─ @types/validator@13.11.9
├─ @vinejs/compiler@2.4.0
├─ @vinejs/vine@1.7.1
├─ arktype@1.0.29-alpha
├─ camelcase@8.0.0
├─ dayjs@1.11.10
├─ dlv@1.1.3
├─ joi@17.12.1
├─ just-clone@6.2.0
├─ memoize-weak@1.0.2
├─ normalize-url@8.0.0
├─ property-expr@2.0.6
├─ superstruct@1.0.3
├─ sveltekit-superforms@2.3.0
├─ tiny-case@1.0.3
├─ toposort@2.0.2
├─ ts-deepmerge@7.0.0
├─ validator@13.11.0
├─ yup@1.3.3
├─ zod-to-json-schema@3.22.4
└─ zod@3.22.4
ciscoheat commented 9 months ago

All packages are installed, but only the ones used will be bundled. What happens if you uninstall zod-to-json-schema?

vladshcherbin commented 9 months ago
image

Gives an error since I don't use the package directly

ciscoheat commented 9 months ago

I don't get the warning when building, so if you have a repo that reproduces it, I'll take a look.

vladshcherbin commented 9 months ago

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

ciscoheat commented 9 months ago

As mentioned in the other issue, it doesn't happen when using pnpm. Haven't used yarn (more than when absolutely necessary), so maybe it can be configured to handle it.

vladshcherbin commented 9 months ago

This warnings are coming from rollup, I've seen them before in other projects - issue https://github.com/rollup/rollup/issues/1089 and many other issues/questions in google. They can be configured there (even mentioned in rollup docs) but rare libraries throw them.

Surprising pnpm doesn't have them (or somehow suppress), the bundler is the same 🙈

rudiv commented 8 months ago

So when using pnpm this issue still appears for me, likewise the vinejs issue (in #351) with @dns/promises.

The build time is greatly increased here too (without super forms, 46sec for this project and with, 1min 52sec) as it does appear to be bundling them in the first stage, even if they were removed in the second.

I'm probably being very stupid, but I too don't see a way to remove these "optional" dependencies, they're in the pnpm-lock with the tag of optional: true, but is very much still installed and there. Using ppm install --no-optional seems to break the build entirely and shouldn't be required:

#10 17.89 .../esbuild@0.19.12/node_modules/esbuild postinstall$ node install.js
#10 18.25 .../esbuild@0.19.12/node_modules/esbuild postinstall: [esbuild] Failed to find package "@esbuild/linux-x64" on the file system
#10 18.25 .../esbuild@0.19.12/node_modules/esbuild postinstall: This can happen if you use the "--no-optional" flag. The "optionalDependencies"
#10 18.25 .../esbuild@0.19.12/node_modules/esbuild postinstall: package.json feature is used by esbuild to install the correct binary executable
#10 18.25 .../esbuild@0.19.12/node_modules/esbuild postinstall: for your current platform. This install script will now attempt to work around
#10 18.25 .../esbuild@0.19.12/node_modules/esbuild postinstall: this. If that fails, you need to remove the "--no-optional" flag to use esbuild.
#10 18.25 .../esbuild@0.19.12/node_modules/esbuild postinstall: [esbuild] Trying to install package "@esbuild/linux-x64" using npm

Isn't just having them as a peerDependency enough so that they're not installed by default? I note that for example @vinejs/vine is both optional and peer.

ciscoheat commented 8 months ago

One minute extra for bundling (and removing) 5-6 extra libraries seems quite strange. The packaging system is complicated, bordering on complex, so removing them from optional causes other issues, resulting in having to add the "middleman" packages such as the JSON Schema generator (specific for each validation library) also as a peerDependency.

The most convenient has been to keep them in both, and I haven't heard of any build problems until now, so if you can check further what's going on, and be quite certain that it doesn't have to do with anything else in the build, that would help me figure things out.

defalt-91 commented 4 months ago

problem will happen when we use @sveltejs/adapter-node

rushkii commented 3 months ago

problem will happen when we use @sveltejs/adapter-node

yeah, me too.

Rar9 commented 1 month ago

+1 Any update/patch for this issue? Happens with valibot & adapter-node

ciscoheat commented 1 month ago

I hope splitting the adapters into separate packages will improve things in v3. But again, the build/package system is complicated so the problem may lie elsewhere.

nikonikoniko commented 3 weeks ago

We also have this issue, and it is causing build issues. If I manualy pin the zod-to-json-schema version in package.json to "3.23.3" - then I do not get the circular dependency any more.

Removing the optional dependency resulted in other build errors, unfortunately. We do not use zod-to-json-schema anywhere in our code.