Upgrade SvelteKit

[AndrewLester]

Status:

🚀 Ready

Description

Upgrades SvelteKit version to fix prefetching.

Fixes: #55
Fixes: #56

Screenshots

Ignore the errors, I can't connect to the API.

Screen.Recording.2022-03-17.at.12.41.48.PM.mov

[SirajChokshi]

Thanks for maintaining this.

The pre-fetching is great, but it seems like the mobile navigation toggle is broken (https://deploy-preview-57--h4iuiuc.netlify.app/)

[AndrewLester]

Thanks for maintaining this.

The pre-fetching is great, but it seems like the mobile navigation toggle is broken (https://deploy-preview-57--h4iuiuc.netlify.app/)

That's unfortunate, the preview deployments really come in handy. After checking the console, I think some script is erroring out and it's crashing Svelte before it can register the button on:click listeners. Here's a screenshot of the error's location in the script, looks like it might be ackee?

Here's a screenshot of a potential root cause (note the {}.CLIENT_ACKEE_URL, which yields undefined):

You might have a better idea of why this is happening, I'm not entirely sure though. Maybe the Ackee urls aren't in the preview?

[arpanlaha]

Thanks for maintaining this.
The pre-fetching is great, but it seems like the mobile navigation toggle is broken (https://deploy-preview-57--h4iuiuc.netlify.app/)

That's unfortunate, the preview deployments really come in handy. After checking the console, I think some script is erroring out and it's crashing Svelte before it can register the button on:click listeners. Here's a screenshot of the error's location in the script, looks like it might be ackee?

Here's a screenshot of a potential root cause (note the {}.CLIENT_ACKEE_URL, which yields undefined):

You might have a better idea of why this is happening, I'm not entirely sure though. Maybe the Ackee urls aren't in the preview?

Ackee was me, I'll take a look soon (probably the weekend).

[AndrewLester]

Coming back to this, it might just be better to upgrade SvelteKit to its most recent version since a lot has changed. The SvelteKit docs really only describe its most recent version, so it will be easier to maintain with a newer version in the codebase. I've recently done a SvelteKit upgrade, so I know generally what needs to be changed in the codebase and I could handle it in this PR. Thoughts?

[arpanlaha]

Coming back to this, it might just be better to upgrade SvelteKit to its most recent version since a lot has changed. The SvelteKit docs really only describe its most recent version, so it will be easier to maintain with a newer version in the codebase. I've recently done a SvelteKit upgrade, so I know generally what needs to be changed in the codebase and I could handle it in this PR. Thoughts?

Sure go for it, sorry I forgot to follow up earlier. Let me know once you've bumped to a more up-to-date version and I'll take a look again.

[AndrewLester]

This PR unfortunately became quite large because of the upgrade. Here's a rundown of why I made some changes that increased the files changed by a lot:

1. Removing the custom import alias for $lib (was pointing at src). I removed this because it was messing up SvelteKit's new generated types system. In doing so, I added new import aliases which resulted in almost every one changing.
2. More explicit typing of endpoint and page load functions. Explicitly typing these functions allows SvelteKit's generated types to work when necessary, meaning page params are typed. I wanted to make this consistent so I typed every one, changing all of these functions to arrow functions in the process. Since not all of them made use of SvelteKit's generated types, for some I just imported their type from the normal SvelteKit type.
3. Changed type definitions from interface to type. I made this change because of an issue I came across before when returning interface defined types in endpoint functions. For some reason, SvelteKit's type system doesn't think interface defined types are JSON compatible. Swapping to type fixes this issue. This TS playground displays the issue.

Also, SvelteKit now requires node 16 I believe. Netlify might need to upgrade? I've never used it though so I'm not entirely sure if I'm reading the logs right.

Lastly, I'm going to make this a draft until I get the contentful API keys and can test locally.

[SirajChokshi]

Thanks for all the work on this @AndrewLester 🚀

I think @arpanlaha has access to the Netlify setup, there should be an option to upgrade to Node 16 LTS.

[arpanlaha]

I upgraded Node, here's the deploy preview: https://deploy-preview-57--h4iuiuc.netlify.app/ (doesn't display on CI since I went through the netlify thing itself)

[arpanlaha]

I added the Ackee env vars to the preview Netlify and we still get a network error (Ackee needs to specify a specific domain which we can't do with preview deploys) but IIUC it isn't blocking the onclick handling now. @AndrewLester could you confirm?

[arpanlaha]

Thanks for doing this, some feedback that is either relatively small or non-blocking:

[arpanlaha]

Sweet

[AndrewLester]

One last change I made was to separate the layout file into two, __layout-root.svelte and [email protected]. The root layout file can be inherited directly by any page to "reset" the layout without missing necessary stuff like ackee (and global CSS in the future). The default layout file that inherits from root has all the actual stuff for page layout like the nav and <main>.

[SirajChokshi]

Great work 💯