Update Solid ecosystem package logic to include packages with peerDep… #6934
Conversation
🦋 Changeset detectedLatest commit: 80d447f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: solid
| @@ -8,7 +8,7 @@ export async function getSolidPkgsConfig(isBuild: boolean, astroConfig: AstroCon | |||
| isBuild, | |||
| viteUserConfig: astroConfig.vite, | |||
| isFrameworkPkgByJson(pkgJson) { | |||
| return containsSolidField(pkgJson.exports || {}); | |||
| return containsSolidField(pkgJson.exports || {}) || !!(pkgJson.peerDependencies || {})['solid-js']; | |||
There was a problem hiding this comment.
The logic here is a tiny bit hard to read with the numerous pipes. Reviewing this, I was actually confused that the code added was in the parameter of containsSolidField and it took me a few reads before getting the logic and that it wasn't a parameter. Maybe a comment could help?
I normally wouldn't bother, but since we get comments after not understanding external stuff often, I figure it's worth having a comment
There was a problem hiding this comment.
Yeah, that's very true. I'm actually going to hold off on this one until I have a chance to speak with @bluwy because maybe making all packages with a peerDep isn't the right call here. Might just want to hardcode this one instead.
There was a problem hiding this comment.
We'd probably want to align with vite-plugin-solid's implementation too, so I think we should avoid this as the package would also have issues in other Vite SSR Solid setups.
Maybe a solution is to remove the solid-js string here:
Or only apply it in dev.
There was a problem hiding this comment.
Sounds reasonable since that's what vite-plugin-solid does. I've update the PR to remove 'solid' as a noExternal, seems to work from my testing.
.changeset/purple-lamps-sleep.md
Outdated
| "@astrojs/solid-js": patch | ||
| --- | ||
|
|
||
| Update Solid ecosystem package logic to include packages with peerDep of Solid |
There was a problem hiding this comment.
This probably needs to be changed with the new PR changes.
… of Solid
Changes
Testing
Tested in the example project
Docs
N/A