Resolve components by module ID during compilation #3300
Conversation
🦋 Changeset detectedLatest commit: e523a0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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: astro
tony-sull
force-pushed
the
fix/hydration-in-npm
branch
3 times, most recently
from
4359e72 to
2282d0b
Compare
| viteTransform: TransformHook; | ||
| } | ||
|
|
||
| async function compile({ |
There was a problem hiding this comment.
(opinionated) using a single object prop to clean up the method props
tony-sull
force-pushed
the
fix/hydration-in-npm
branch
from
95a8e27 to
afaa67e
Compare
|
Thank you @tony-sull really appreciate this fix dude |
|
Dang, was really hopeful this might also solve #3053, but it doesn’t look like it does. Great work nevertheless! |
| "@astrojs/preact": "workspace:*", | ||
| "@astrojs/react": "workspace:*", | ||
| "@astrojs/svelte": "workspace:*", | ||
| "@test/component-library-shared": "workspace:*", |
There was a problem hiding this comment.
I'm always a little weary of workspace:* as a way to test external dependencies. Is it possible to generate a tarball from component-library-shared via pnpm pack, and reference that file from here? This would 100% mock an external npm dependency
There was a problem hiding this comment.
I'm not actually sure if tarballs work properly in vite 🤔 I don't use pnpm pack often so I may have it misconfigured, but I tried this locally and Vite blew up with errors failing to resolve anything inside the tarball dependency
link:../component-library-shared would work for the tests, but I'm not sure the details of how that's different from workspace:*
|
Also, does this resolve any reported bugs? Didn't see any in the description so double checking! |
|
@bholmesdev I still need to make a pass through all the open issues to see if any seem related. I can't help but think this was the underlying cause of a few strange bugs, but I ran into this when working on the docs template and forgot to file my own issue to track before diving into the fix |
I was hoping the same 🙃 Hopefully this gets us closer though, I'd really like to see that issue fixed soon as well! |
tony-sull
force-pushed
the
fix/hydration-in-npm
branch
from
afaa67e to
e523a0c
Compare
* WIP: adding test coverage * test fixes * moving the shared lib up a directory to reproduce the bug * fix: transform with the module ID instead of parsing the filepath * adding the shared lib to the workspaces list * fix: client-only assets now get the full URL from vite * why is this needed for windows? * WIP: using /@fs to handle windows filepaths * fix: remove /@fs from hoisted script imports * nit: removing unused imports * fix: strip off the path root when mapping client:only styles * had to reverse the `/@fs` handling to work on windows and unix * chore: adding comments to explain the fix * chore: adding changeset
Changes
This updates Astro to resolve
.astrocomponents by Vite's module ID, fixing an issue using.astroand framework components in shared NPM packagesDetails
During the compilation step, Astro was parsing the file path for a component and expecting astro components to always live in the project's root directory.
This caused a few weird corner cases for NPM libraries that use both
.astroand framework components internally, ex: a theme that is mostly.astrowith a few internal react components for interactivity.Testing
Added a test suite that uses components from a separate package in the monorepo. Without the fix this test suite fails to build when the component files can't be found
Docs
None, but I will open a separate PR to update our
componentstarter once this fix is published