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

In adapter-netlify split:true config is breaking SSR and other things. #4872

Closed
brittneypostma opened this issue May 11, 2022 · 14 comments · Fixed by #5180
Closed

In adapter-netlify split:true config is breaking SSR and other things. #4872

brittneypostma opened this issue May 11, 2022 · 14 comments · Fixed by #5180
Labels
Milestone

Comments

@brittneypostma
Copy link
Contributor

brittneypostma commented May 11, 2022

Describe the bug

When split: true is enabled in the svelte.config.js for adapter-netlify, SSR routes when navigated to with client side navigation gets a 404. The endpoint route will work on refresh, but the todo page goes to a Netlify 404 page does not exist skipping the __error.svelte route.

Reproduction

Repo of demo app with adapter-netlify: https://github.com/brittneypostma/test-ssr-split

Preview deploy of site with split: false and __error.svelte working: https://627c04fe0cebc30008f3f36b--test-ssr-split.netlify.app/endpoint

Preview deploy of site with split: true and __error.svelte not catching on /todos refresh: https://627c046d20bc620009be8eef--test-ssr-split.netlify.app/endpoint

Logs

`GET /endpoint/__data.json 404`

System Info

System:
    OS: macOS 12.3.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 127.41 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.15.0 - ~/.nvm/versions/node/v16.15.0/bin/node
    npm: 8.5.5 - ~/.nvm/versions/node/v16.15.0/bin/npm
  Browsers:
    Chrome: 101.0.4951.54
    Firefox Developer Edition: 101.0
    Safari: 15.4
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.40 
    @sveltejs/adapter-netlify: 1.0.0-next.56 => 1.0.0-next.56 
    @sveltejs/kit: next => 1.0.0-next.326 
    svelte: ^3.46.0 => 3.48.0 

Severity

serious, but I can work around it

Additional Information

Only workaround is to turn off split, which isn't ideal.

@Rich-Harris
Copy link
Member

Ah... yep. This is an unanticipated problem of introducing page endpoints — the platform-level router won't execute the /foo function for /foo/__data.json, because it doesn't think it's a match. In Netlify terms, this line...

redirects.push(`${pattern} /.netlify/functions/${name} 200`);

...probably needs to be accompanied by one with the __data.json suffix.

Currently that would mean adding that redirect for all routes, regardless of whether a given route a) is a page or b) has a corresponding endpoint. Ideally the route in builder.createEntries((route) => {...}) would expose that information. It would be better still if adapters didn't need to be aware of this at all, though I'm not quite sure what that would look like.

A broader point: we really should have some e2e tests that would make it easier to catch these sorts of things.

@Rich-Harris Rich-Harris added bug Something isn't working pkg:adapter-netlify labels May 11, 2022
@Rich-Harris Rich-Harris added this to the 1.0 milestone May 11, 2022
@ericapisani
Copy link
Contributor

Happy to try and tackle this.

From initial testing of the fix that Rich had suggested, it looks like a bit of a timing/race condition issue is at play as well at

builder.createEntries((route) => {

where the Promise returned by createEntries was completing before the entries for todos were pushed onto the redirects array and the array written to the build/redirects file.

Currently that would mean adding that redirect for all routes, regardless of whether a given route a) is a page or b) has a corresponding endpoint. Ideally the route in builder.createEntries((route) => {...}) would expose that information.

From what I was able to find in the docs it looks like the type property for the route parameter within builder.createEntries would be either 'page' or 'endpoint' - are there other values that type could potentially be that are undocumented and that could be seen in routes?

If so, I can add some code to explicitly check for page or endpoint when adding the __data.json route, but at least from what I can see it seems like it's a safe assumption to make that the route will either be a page or an endpoint.

@brittneypostma
Copy link
Contributor Author

Could we open this back up? It looks like we are still breaking something along the way. I upgraded the old site and it still 404's, but a new SvelteKit site gets a 500 error on those SSR routes. On a client side refresh, I can load the endpoint route on the "old" upgraded site, but the new one only gets a 500.

@dummdidumm dummdidumm reopened this Jun 24, 2022
@winston0410
Copy link

Any update on the issue? I am having the same issue, getting 404 even when I havnt set split: true. Basically this prevent running Sveltekit on Netlify right now.

@ericapisani
Copy link
Contributor

Interesting 🤔 I'll take another look at this again and see if I can puzzle out what's happening

@winston0410
Copy link

@ericapisani I have a repo to reproduce this here, I hope it will be helpful to you: https://github.com/winston0410/netlify-repo

@ericapisani
Copy link
Contributor

@brittneypostma

It looks like we are still breaking something along the way. I upgraded the old site and it still 404's

I've forked the old test site that you had created and after upgrading the version of @sveltejs/adapter-netlify to 1.0.0-next.65, the site works correctly (no 404s or 500s when navigating to /todos or /endpoints pages, either directly or through client side navigation).

Deployed test site can be found here

If that site continues to experience issues with that upgrade, could you please specify what steps you're taking to reproduce error?

...a new SvelteKit site gets a 500 error on those SSR routes

I've also forked the new SvelteKit site that you mentioned above, and it appears that the 500 error that you're mentioning is due to missing import. In this file the following line is throwing a TypeError:

event.locals.userid = cookies['userid'] || crypto.randomUUID();

and it's due to crypto not being imported. After importing and redeploying that test site appears to work as expected

@winston0410 I was just going to ask about a repro project 😄 Thank you so much, I'll take a look now

@ericapisani
Copy link
Contributor

@winston0410 Just deployed the repro project you had provided by following the instructions in the README, and it appears to be deploying correctly with no 404 on the home page - is there a step that I'm missing to reproduce the error?

@winston0410
Copy link

@ericapisani Have you set split: false again? I think I have set it back to true, to verify it can deploy on my final commit, maybe that's why it deployed correctly

winston0410/netlify-repo@aeccfd3

@ericapisani
Copy link
Contributor

@winston0410 Ah, that seems to have done the trick, thank you! Will continue investigating 🕵🏻‍♀️

@ericapisani
Copy link
Contributor

@winston0410 I think this is a cache issue of some kind. When I built and deployed the project with split: true and then changed it back to false without deleting the build directory, that's when I encounter the 404 error.

When I removed the build directory between configuration changes and then run ntl deploy --build --prod things worked as expected. I've confirmed that all of the below scenarios deploy and run successfully:

Test 1: Defaults

adapter: adapter(),

Test 2: Explicitly define default values

adapter: adapter({
	edge: false,
	split: false
}),

Test 3: Set split to true

adapter: adapter({
	edge: false,
	split: true
}),

Would you mind giving that a try (deleting the build directory) and see if that resolves your issue?

@winston0410
Copy link

@ericapisani I have just tried it again, and cache is the issue, that's a tricky one to catch! And I can confirm that after I have removed all the cache, split:true would work

@brittneypostma
Copy link
Contributor Author

Thanks for the updates on this. I was on vacation and just getting back and catching up. I will run these when I jump on. There is another ongoing issue with SvelteKit building on Netlify and want to make sure this change isn't causing that error as well. It is closed, but not fixed so I've got some digging to do. #5337

@Rich-Harris
Copy link
Member

It sounds like we can close this again — thanks all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants