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

fix windows package export #4141

Merged
merged 2 commits into from
Aug 4, 2022
Merged

fix windows package export #4141

merged 2 commits into from
Aug 4, 2022

Conversation

FredKSchott
Copy link
Member

Background

Update on the astro/dist import problem: This may be bigger than we realized, it seems to be impacting all Windows users in two different use-cases:

  • injectRoute + entryPoint: @astrojs/image/endpoints/dev
  • renderers + serverEntrypoint: astro/jsx/server.js

From some digging, I think this is a Vite 3 issue. The one thing that both of these have in common is that they are both eventually passed already-resolved to viteServer.ssrLoadModule(id) in our codebase. I think this is what causes the error on Windows, where Vite tries to resolve id itself via ensureEntryFromUrl and fails because its resolved form is not a valid package export.

None of the other framework renderers hit this because their export maps are 1:1 with the file location. ex: "./server.js": "./server.js",

Changes

  • Short-term fix to the issue is to just add those resolved dist/* paths to the export map so that they become valid imports even in their resolved form.
  • Long-term fix is to investigate why this is happening on Windows only, and how to fix. If we want to pass the unresolved id to ssrLoadModule() some refactoring will be needed on our end.

@matthewp and @bluwy -- this could be an interesting one to dig into together to at least figure out what Vite is expecting in this case, and maybe why Vite is only erroring on Windows.

Testing

  • Tests are covering this so I'm not sure why they are passing on Windows.
  • I'm going to try to break tests in a different PR, maybe this is only happening on Node v16+

Docs

  • N/A

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2022

🦋 Changeset detected

Latest commit: 002ceef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
astro Patch
@astrojs/image Patch
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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 github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) labels Aug 4, 2022
@FredKSchott
Copy link
Member Author

This seems to be breaking more people than we realized. Given that this is a fairly trivial manifest change with a big impact on RC users, I'd like to merge and release this tonight.

@FredKSchott FredKSchott merged commit 65f2d3b into main Aug 4, 2022
@FredKSchott FredKSchott deleted the fix-windows-import-1 branch August 4, 2022 06:11
@astrobot-houston astrobot-houston mentioned this pull request Aug 4, 2022
@FredKSchott
Copy link
Member Author

Shout out to @crutchcorn for also finding this short-term workaround on Discord!

@bluwy
Copy link
Member

bluwy commented Aug 4, 2022

Might be related to vitejs/vite#8420. It should be fixed though, but perhaps Windows had a special case that still causes it.

@bluwy bluwy assigned bluwy and unassigned bluwy Aug 6, 2022
@bluwy
Copy link
Member

bluwy commented Aug 9, 2022

I tried looking into this today in both Vite and how Astro uses ssrLoadModule and I can't seem to find anything that would be a problem for Windows only (I don't have a Windows machine to test out at hand).

Logging around on macos, it seems that we're always only loading either astro/jsx/server.js or the full path /Users/bjorn/Work/repros/astro-image-ssr/node_modules/.pnpm/@[email protected]/node_modules/@astrojs/image/dist/endpoints/dev.js. The error seem to only happen if we load @astro/image/dist/endpoints/dev.js instead.

@crutchcorn Do you have a repro and full stack trace to look at for the error? I think that'll help in case I'm debugging the wrong thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants