Skip to content

fix: use build.client output directory for adapters #64

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 4 commits into from
Apr 10, 2025

Conversation

hugo-vrijswijk
Copy link
Contributor

For server outputs, the dist/client output directory should be used and not dist.

@hugo-vrijswijk hugo-vrijswijk changed the title fix: use build.client config dir if available fix: use build.client config for server output Apr 2, 2025
@userquin
Copy link
Member

userquin commented Apr 2, 2025

what's the issue?

@hugo-vrijswijk
Copy link
Contributor Author

On any configuration, all pwa-assets will be output to dist/ instead of dist/client/ for server output. For static sites, this is what you want, but for server output the files will not be found (Astro will only look in dist/client for static assets on server output)

For server outputs, the `dist/client` output directory should be used and not `dist`.
@hugo-vrijswijk hugo-vrijswijk changed the title fix: use build.client config for server output fix: use build.client output directory for adapters Apr 2, 2025
@hugo-vrijswijk
Copy link
Contributor Author

Ok slightly different. The detection should be if config.adapter is used, not config.output. Since you can use an adapter with static output. But if an adapter is used, the outDir should be dist/client

@userquin
Copy link
Member

userquin commented Apr 2, 2025

Looks like we also have adapters in Astro ;)

Can you send a new PR with an SSR minimal example (or just include it here) 🙏?

EDIT: I need some time to read the Astro docs

@hugo-vrijswijk
Copy link
Contributor Author

I'll add a SSR example

@hugo-vrijswijk
Copy link
Contributor Author

@userquin I've pushed a SSR example. It's mostly the same code as examples/pwa-simple-assets-generator. But the asset generation is needed to show the bug.

Copy link

pkg-pr-new bot commented Apr 2, 2025

npm i https://pkg.pr.new/@vite-pwa/astro@64

commit: d321f2b

@userquin
Copy link
Member

userquin commented Apr 2, 2025

The example will not work when going offline, the sw requires the navigate fallback (offline support) and since we're not prerendering (SSG for at least 1 route => the offline page) any route will fail, but the example is fine.

With any SSR (meta-)framework we have previous problem, I need to think the best solution for this to solve the problem in all integrations, for context:

Looks like we can prerender routes: https://github.com/withastro/astro/blob/3b10b97a4fecd1dfd959b160a07b5b8427fe40a7/packages/astro/src/types/public/integrations.ts#L26

@userquin
Copy link
Member

userquin commented Apr 2, 2025

@hugo-vrijswijk added prerendering index route and added small runtime caching to cache SSR pages (will use always NetworkFirst strategy), this way if the client navigates the application and goes offline, the cache will allow navigate precached routes (visited), otherwise error.

EDIT: looks like Astro won't have solution for this since there is no runtime in the client (client router) like Nuxt or SvelteKit.

Copy link
Member

@userquin userquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx ❤️

@hugo-vrijswijk
Copy link
Contributor Author

@userquin can this be merged? Or does it need more changes?

@userquin
Copy link
Member

userquin commented Apr 9, 2025

Yes, I'll merge and release a patch version in the afternoon, I was asking Astro people (Nate) about partial SSR support (there is no client-side runtime).

Did you check the changes in the example?

@userquin userquin merged commit b605dbe into vite-pwa:main Apr 10, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants