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

Build-time circular dependency warnings #1067

Closed
2 tasks done
rolanday opened this issue Sep 23, 2023 · 2 comments
Closed
2 tasks done

Build-time circular dependency warnings #1067

rolanday opened this issue Sep 23, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@rolanday
Copy link

rolanday commented Sep 23, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

I'm seeing a flood of circular dependency warnings when building solid-start project. For example:

Circular dependency: node_modules/ioredis/built/Redis.js -> node_modules/ioredis/built/cluster/index.js -> node_modules/ioredis/built/Redis.js
Circular dependency: node_modules/ioredis/built/Redis.js -> node_modules/ioredis/built/cluster/index.js -> node_modules/ioredis/built/cluster/ClusterSubscriber.js -> node_modules/ioredis/built/Redis.js
Circular dependency: node_modules/ioredis/built/Redis.js -> node_modules/ioredis/built/cluster/index.js -> node_modules/ioredis/built/cluster/ConnectionPool.js -> node_modules/ioredis/built/Redis.js
Circular dependency: node_modules/ioredis/built/Redis.js -> node_modules/ioredis/built/connectors/index.js -> node_modules/ioredis/built/connectors/SentinelConnector/index.js -> node_modules/ioredis/built/Redis.js

This does not appear to be a Vite issue as I can build similar for Qwik, which builds cleanly:
https://github.com/rolanday/solid-app-circular-dep-issue <== (build warnings)
https://github.com/rolanday/qwik-app-circular-dep-issue <== (builds clean)

Does not affect function afaict; however, these are just sample projects I created for filing issue. Real project with other deps (+kysely, +aws-*) results an warnings throwing up all over my screen, which is very unsettling and not something I'd be OK taking an app into production with.

There is also a single warning for virgin project using recommended starter command:

node_modules/solid-start/islands/A.tsx (1:0) Module level directives cause errors when bundled, "use client" in "node_modules/solid-start/islands/A.tsx" was ignored.

Expected behavior 🤔

Builds clean

Steps to reproduce 🕹

Steps:

  1. Clone project links provided and build using bun run build
  2. Observe output of solid v. qwik build (both use Vite)

Context 🔦

No response

Your environment 🌎

MacOS 13.5.2 (22G91), a.k.a., Ventura
Node v20.6.1
@rolanday rolanday added the bug Something isn't working label Sep 23, 2023
@rolanday
Copy link
Author

rolanday commented Sep 24, 2023

Update. I was able to clean-up the warnings by adding ssr.extenal entries to my vite.config.ts (see below). Not clear to me why same is not needed for qwik build (I even matched-up vite versions, and configs modulo respective solid & qwik plug-ins) so must be getting handled automagically somehow by qwik build.

import solid from "solid-start/vite";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [solid()],
  ssr: {
    external: [
      "ioredis",
      "@aws-sdk/client-s3",
      "@aws-sdk/s3-request-presigner",
      "kysely",
    ],
  },
});

Now there is just the node_modules/solid-start/islands/A.tsx (1:0) Module level directives cause errors when bundled, "use client" in "node_modules/solid-start/islands/A.tsx" was ignored. warning, which I assume is known issue since happens with clean setup using bunx create-solid.

@ryansolid
Copy link
Member

Yeah.. it is known. This behavior is just Vite behavior. It's possible Qwik already has redis in an ignore list but this is on the Vite side in terms of resolution.

In setting up for SolidStart's next Beta Phase built on Nitro and Vinxi we are closing all PRs/Issues that will not be merged due to the system changing. If you feel your issue was closed by mistake. Feel free to re-open it after updating/testing against 0.4.x release. Thank you for your patience.

See #1139 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants