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

Build Fails with Dynamic Routes #1

Open
dl-eric opened this issue Feb 28, 2024 · 5 comments
Open

Build Fails with Dynamic Routes #1

dl-eric opened this issue Feb 28, 2024 · 5 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@dl-eric
Copy link

dl-eric commented Feb 28, 2024

We are looking to use this adapter to build our PWA, but we have dynamic routes that we'd traditionally need to specify a fallback page for. But we have no way to specify it because Adapter-Versioned-Worker does not expose those to us.

It would be helpful if this adapter's config includes a bit where we can specify adapter-static settings, which would then be consumed:

export function adapter(inputConfig: AdapterConfig): Adapter {
...
	const config = applyAdapterConfigDefaults(inputConfig);
	adapterConfig = config;

	const adapterInstance = adapterStatic({ pages: adapterConfig.outputDir, fallback: <>, strict: <> });
...
}

Please consider, and I'm happy to create a PR if needed. Thank you!

@hedgehog125
Copy link
Owner

Thanks for your interest, I think this is the first non-trivial issue I've got from someone I don't know 😄.

I originally decided against adding support for dynamic routes as I thought it required SSR and I don't think service workers are that beneficial in that case (since you probably can't add an offline mode). However, I just looked into fallback pages and it looks like it's worth supporting since there's no SSR to complicate things. If I'm understanding you correctly, you're using this in your top level +layout.ts?

export const prerender = false;
export const ssr = false;

I haven't worked on this package in a while so I've forgotten some things but I do remember that there being a set number of routes is an assumption that the code makes in quite a few places. The code currently also isn't set up to handle special routes like the fallback page. Anyway, I had a look through the code and made a list of (at least some of the) things that depend on these assumptions. They all seem to be in the worker:

  • The constant TRAILING_SLASH is set based on if any of the ROUTES contain a trailing slash. This should probably just become an export from the worker virtual module, which already contains the ROUTES export.
  • The worker's redirection of trailing slashes involves checking if the request is for a route. SvelteKit seems to handle the redirect itself rather than the server when you navigate to a route that uses a fallback page so if ROUTES only contains the prerendered pages, then this might not need changing.
  • fetchResource should maybe be updated so it can display the "connect to the internet" message for fallback routes. But it's only supposed to display that when the cache storage is tampered with anyway, so it might not be worth changing.

And a few other things that would need to be changed:

  • The inCacheList variable should probably be false for dynamic routes. This is used in a few places including being passed to the handleFetch hook so some changes might need to be made.
  • The fallback page will need to be added to toDownload in the worker with each update.
  • The worker will also need to send the fallback page if that's enabled and there's no cache match.
  • The fallback page probably shouldn't be in ROUTES or any of the cache lists. Without changes I think it'll probably end up being sorted like a file into one of the cache lists but not included in the ROUTES.


Unfortunately I don't have much time to work on this package anymore as I've now got more obvious gaps to fill in my portfolio. I might add some proper tests though at some point, depending on what happens with my career.

However, if you're up for trying to add this, I'd be very happy to be a maintainer and I might be able to lend a hand with a few things. I think I've laid some good groundwork here so it should be significantly easier than making something from scratch. But if you're just trying to get something working quickly, you might have better luck using Workbox with Vite PWA (although I kind of made this package out of frustration trying to use them 😅).

Any thoughts?

@hedgehog125
Copy link
Owner

I also just updated the TODO.md file. That top bug fix might as well go into the update as it should only involve changing a couple of lines.

@hedgehog125
Copy link
Owner

hedgehog125 commented Feb 29, 2024

Just thought of one other problem: the worker currently tries the network by default if it can't find the requested resource, which is a behaviour you might want to rely on sometimes. The problem is that's not compatible with fallback routes if the device is offline. I thought of 2 possible fixes:

  • The ignored files are provided to the worker so if there's no cache match and either one of those ignored files or any other known file is requested, the network is used rather than the fallback. This would mean the worker can properly emulate the fallback aspect of the server initially, but there'd be a disparity if the worker is out of date or if a new file was added to the server without an update.
  • For navigations, the fallback is just sent if there's no match. This would break navigating to ignored resources but since they aren't pages, they probably aren't meant to be directly accessed. It would also create a disparity when fetching a route, but that's also probably not that important.

And I suppose a 3rd would be to try the network with a timeout and a fallback, but that would unnecessarily slow down the first load.

Edit: Also, some similar changes might have to be made for the other request modes, but since they aren't used for navigations (except with the search param if someone wants to do that for some reason), they might not need changing.

@dl-eric
Copy link
Author

dl-eric commented Mar 19, 2024

Hey, thanks for your quick and thorough reply! I didn't realize how much of this adapter relied on having prerender = true configured. Indeed, our top-level +layout.js file has:

// Set ssr and prerender to be false to compile this as a full SPA app.
export const ssr = false;
export const prerender = false;

Reading all the considerations you laid out that must be met in order to support fallback pages, I also won't have time to work on this despite my initial offer :( For now, we aren't using this adapter anymore to unblock ourselves. But I do wish this project the best - certainly the groundwork is there!

@hedgehog125
Copy link
Owner

Ah ok. Well, thanks for bringing this to my attention.

@hedgehog125 hedgehog125 added enhancement New feature or request help wanted Extra attention is needed labels Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants