Skip to content

fix: ensure solid-js is included in pre-bundle #12

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

Merged
merged 1 commit into from
May 13, 2021

Conversation

jorroll
Copy link
Contributor

@jorroll jorroll commented May 13, 2021

This PR is a necessary component of fixing solidjs/solid-start#4.

In a standard, single project repo, running the vite dev server with this plugin (via solid-start) results in three dependencies being pre-bundled: solid-js/web, solid-js, and solid-js/dev even though this plugin only specifies optimizeDeps.include: ['solid-js/dev', 'solid-js/web'].

However, in a monorepo setup only two dependencies are pre-bundled: solid-js/web and solid-js/dev. This results in the following error during development (I never tested production) and the development build is never served

You appear to have multiple instances of Solid. This can lead to unexpected behavior.
computations created outside a `createRoot` or `render` will never be disposed

By adding solid-js to optimizeDeps.include, the You appear to have multiple instances of Solid... errors are eliminated and, in my testing, there appears to be no affect for single project repositories. I believe this PR is a necessary component of supporting monorepo development (the end user can also work around this issue by manually adding optimizeDeps.include: ['solid-js'] to their local vite.config.ts file, but this bakes monorepo support into the plugin itself).

@jorroll
Copy link
Contributor Author

jorroll commented May 13, 2021

Note, addressing solidjs/solid-start#4 also involved another PR, solidjs/solid-start#5, which was just merged and triggered the closing of solidjs/solid-start#4. However, solidjs/solid-start#4 hasn't actually been fixed until this PR is merged.

@ryansolid
Copy link
Member

Yeah this one makes a lot of sense to me. Not sure if there was a reason for the exclusion. @amoutonbrady the original author will be around in a couple hours. And will ask him then.

@amoutonbrady
Copy link
Member

Yeah I'm not sure. I think this was because at some point vite was totally able to optimize the solid-js imports since it's just typescript stuff.

I'll merge this, it certainly can't hurt. Thansk!

@amoutonbrady amoutonbrady merged commit ba5d40c into solidjs:master May 13, 2021
@amoutonbrady
Copy link
Member

This is available in 1.8.0 🎉

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

Successfully merging this pull request may close these issues.

3 participants