Fix Deploys: Move all Logic from load to onMount
#80
Conversation
|
Hmm ya there are some issues with this approach When I open Settings and then come back I get the error about needing a Which kinda makes sense if Edit: I think this is fixed in my latest commit, by making the back to game button use |
…arams. e.preventDefault seems to work here
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - run: npm ci | ||
| - run: npm run build |
There was a problem hiding this comment.
Added this so that we know if future PRs are going to break builds, even if others tests pass
| const url = new URL(window.location.href); | ||
| settings = loadSettingsWithURLOverrides(url); | ||
|
|
||
| setTheme(settings.theme); | ||
|
|
||
| if (settings.game.length > 0 && settings.engine.length > 0) { | ||
| settingError = false; | ||
| playbackState.load(fetch, settings); | ||
| initWindowMessages(); | ||
| } |
There was a problem hiding this comment.
This is basically what was in load before
Moved it to onMount to quarantee it runs only in the browser, so we can use window.location.href to get the current url, with searchParams!
| @@ -63,7 +80,7 @@ | |||
| /> | |||
|
|
|||
| <div use:resize={{ f: onResize }} class="h-full w-full flex flex-col items-center justify-center"> | |||
| {#if data.settingError} | |||
| {#if settingError} | |||
There was a problem hiding this comment.
These no longer come from data they are 'top level' to this component!
| @@ -7,7 +7,9 @@ | |||
| { value: 18, text: "Fast" } | |||
| ]; | |||
|
|
|||
| function navigateBack() { | |||
| function navigateBack(e: MouseEvent) { | |||
| e.preventDefault(); | |||
There was a problem hiding this comment.
Without this when I left settings it seemed to actually hit the href which didn't have the game query param
Adding this allowed me to go back and forth between the settings page and the game page
Builds are currently failing with this error message
It was introduced in #79
This error is because we are trying to access
url.searchParamsin theloadfunction and ALSO have preloading enabled.Since
loadgets called on the server to pre-render, it doesn't allow us to accessurl.searchParamssince that can only be known in the browserBut this must be new behavior, since it was working for us previously. I think it was working previously is because
loadwas acting as a 'universal` loader and running both in the 'server' when preloading, and also in the client when mounting for the first time.https://kit.svelte.dev/docs/load#universal-vs-server-when-does-which-load-function-run
I imagine the server load was getting an empty searchSet, but continuing to work. And then the client load correctly filled in the query param values
But from reading the Changelog I can't find anything that seems obviously like it would have broken this behavior
I do think this is a good change to prepare for v2, since it seems required there as well!