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

static adapter failing builds when route ends with .html #8676

Closed
john-michael-murphy opened this issue Jan 23, 2023 · 15 comments
Closed

static adapter failing builds when route ends with .html #8676

john-michael-murphy opened this issue Jan 23, 2023 · 15 comments
Labels
breaking change bug Something isn't working p3-edge-case SvelteKit cannot be used in an uncommon way
Milestone

Comments

@john-michael-murphy
Copy link

Describe the bug

Consider this app with one route: potato.html, where .html is explicitly defined in the url:

.
└── src/
    └── routes/
        └── potato.html/
            ├── +page.svelte
            └── +page.server.js

When prerendering server-side only pages with adapter-static, the following error is always encountered:

error during build:
Error: Cannot create directory build/potato.html, a file already exists at this position
    at mkdirp (file:///bug-repro/node_modules/@sveltejs/kit/src/utils/filesystem.js:11:11)
    at go (file:///bug-repro/node_modules/@sveltejs/kit/src/utils/filesystem.js:58:4)

Reproduction

Create a skeleton sveltekit project.
Create a route and explicitly define the .html extension in the route.
Add a server side-only page route.
Try to prerender and build that page with static-adapter.

Logs

No response

System Info

System:
    OS: macOS 12.6.2
    CPU: (10) arm64 Apple M1 Max
    Memory: 3.96 GB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.11.0 - ~/bin/nvm/versions/node/v18.11.0/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 8.19.2 - ~/bin/nvm/versions/node/v18.11.0/bin/npm
  Browsers:
    Chrome: 109.0.5414.87
    Firefox: 109.0
    Safari: 16.2
  npmPackages:
    @sveltejs/adapter-static: 1.0.5 => 1.0.5 
    @sveltejs/kit: 1.2.3 => 1.2.3 
    svelte: ^3.54.0 => 3.55.0

Severity

serious, but I can work around it

Additional Information

No response

@dummdidumm
Copy link
Member

This is a duplicate of #7244 but I closed that issue in favor of this one because of the better title and clear reproduction.

@dummdidumm dummdidumm added bug Something isn't working p3-edge-case SvelteKit cannot be used in an uncommon way labels Jan 23, 2023
@john-michael-murphy
Copy link
Author

Thanks so much, @dummdidumm!

@Rich-Harris
Copy link
Member

Just spitballing, but one possible solution might be to do something like this:

-/some-route/__data.json
+/_data/some-route.json

IIRC the only real reason we use a suffix rather than a prefix is that it's more convenient to inspect the output on disk if it's colocated, and marginally easier to inspect in a browser by appending something to the URL rather than the alternative. But I think this bug probably trumps the convenience argument, given that we basically recommend +page.server.js as the way to load data.

Note that it's /_data rather than /_app/data because config.kit.appDir is relative to paths.assets, whereas this would need to be relative to paths.base (if your assets were served from https://static.example.com/my-app then you wouldn't be able to dynamically generate responses from https://static.example.com/my-app/_app/data/...). Presumably it would need to be configurable in case someone need a /_data route of their own. (Would this need to be treated as a breaking change?)

@john-michael-murphy would that work in your case or would the prerendered data need to come from static01.nytimes.com?

@Rich-Harris Rich-Harris added this to the soon milestone Jan 27, 2023
@john-michael-murphy
Copy link
Author

john-michael-murphy commented Feb 6, 2023

@Rich-Harris we're actually rendering on alpha preview via a whole new mechanism we devised. We can totally work around this issue for now by removing the extension.

Cafezinhu added a commit to Cafezinhu/aerton.dev that referenced this issue Feb 14, 2023
@ghost
Copy link

ghost commented Feb 23, 2023

Found the exact same issue, but in a different context.

My Project uses idb to handle the IndexedDB client side. idb expects a indexedDB to be present in the Global scope. Vite does not have that.
grafik

So I installed fake-indexeddb and imported it. Now Vite was fine, But the Browser complained since it already had an indexedDB.

Then I moved the import "fake-indexeddb/auto" to /src/routes/+layout.server.ts.
That worked for both sides, but now the Static-Adapter is complaining about that exact issue.😭

I can't wrap everything in a huge if(browser) for two reasons:

1.) it is the idb module, that throws the error and imports must be made on the top-level

2.) The modules that use idb also need to be compatible with normal Svelte, which has no browser variable or the required import to get it.

@john-michael-murphy
Copy link
Author

@Rich-Harris realizing that I did not totally understood some of the implications of what you were suggesting. It would be awesome if we could locate page data in _data/some-route.json, so we could then do something like _assets/_app and reliably know that data was located in _assets/_app/_data. That data would eventually be uploaded to static01.nytimes.com, but I think (and correct me if there's something I'm missing) we'd be able to rewrite it, since its path starts with _assets.

@donmccurdy
Copy link

donmccurdy commented May 15, 2023

Quick aside, in case someone else runs into this thread for the same reasons I did. When deploying a static, prerendered site to Vercel, you'll likely want to enable the cleanUrls: true option in vercel.json. I'd assumed that was part of either @sveltejs/adapter-vercel or the SvelteKit Preset in the Vercel UI, but it was apparently not. Without that, paths like /foo (without .html suffix) won't load, and when I tried to just switch my routes to require the .html suffix, I hit the issue described here.

@Monadic-Cat
Copy link

Monadic-Cat commented Dec 5, 2023

I have also encountered this issue, in trying to migrate my website to SvelteKit.
In my case, I have a number of pages authored in Pandoc Markdown, and use a +page.server.ts to load built HTML from my existing build system, and embed it in a +page.svelte like this: {@html data.post.content}.

That introduces a number of links to other pages in my site, which SvelteKit promptly scans and builds as well. After successfully prerendering every page of my blog, we reach the finalise step and SvelteKit fails to copy the __data.jsons for each route (e.g., prerendered/dependencies/blog/theorem_proving_01.html/__data.json for blog/theorem_proving_01.html) and notices that the path blog/theorem_proving_01.html is already occupied by a regular file which was placed there by the +page.svelte.

This behavior apparently reproduces with adapter-node as well as adapter-static, I had someone more experienced with SvelteKit than myself dig into it (@Suyashtnt).

With that in mind, I second the suggestion that SvelteKit should place these files (the __data.jsons) in a separate _data directory (a path prefix instead of a path suffix), which on the face of it seems like it would avoid this problem. That would let me move forward with using SvelteKit, without needing to do deep surgery of all my files and bad workarounds in the server in front of them to preserve existing permalinks, which I'm not actually willing to do.

Thanks everyone for your time, and I understand if this isn't top priority for anyone.

@Suyashtnt
Copy link

For some extra context, the issue happens at https://github.com/sveltejs/kit/blob/master/packages/kit/src/core/adapt/builder.js#L192-L195, since they're copied to the same pre-rendered directory

@dummdidumm
Copy link
Member

dummdidumm commented Dec 8, 2023

How would changing this from being a suffix (like _data.json) to a prefix/infix affect things like cookie paths? AFAIK the browser will automatically determine whether or not to add cookies to a certain request, and with anything other than a suffix it wouldn't send it along.
Example: You're on foo/bar/ (note the trailing slash). You're re-requesting data for that path, which would be foo/_data/bar.json. There's a cookie for the path foo/bar, which matches the page path, but not the data request path. The cookie is not sent along, breaking your app.
To support this, our cookie logic would have to get more sophisticated by automatically adding the cookie to both path variants. People who set cookies more manually would have to adjust their code (is that even possible in all cases, i.e. third party cookies / setting them in the browser?).
Since this only affects prerendered pages, and those can't use cookies, maybe there's a way to square this circle somehow? infix for prerendered pages if needed, otherwise _data.json suffix?

@dummdidumm
Copy link
Member

Rich and I just talked about it, we're going to try to implement the _data/foo.json-for-prerendered-only idea.

@benmccann
Copy link
Member

benmccann commented Dec 8, 2023

Somewhat separately, I had been wanting to propose something like handlePost which would be like handle, but called when serving pre-rendered pages. It wouldn't be supported on adapter-static, but would be in the rest of the adapters. This could be helpful in many usecases. E.g. the docs right now are driving me insane because there's a big flash of black and then they switch to white when rendering because we have no good to way to handle the user preferences for dark mode right now. If we added something like handlePost it probably would conflict with the _data/foo.json idea.

The other option for what I want to do might be some sort of partial prerendering, but it feels like that would require entirely rearchitecting how prerendering works and I really don't know how we'd do it exactly.

Edit:
Or maybe this would work: https://www.reddit.com/r/nextjs/comments/xr879i/comment/iqei5in/

@Rich-Harris
Copy link
Member

I'm not following — we already do this, which sets the relevant class name before anything is rendered?

image

I do see a flash when dark mode is enabled, but that seems to be something to do with wonky image dimensions.

Anyway I'm not sure I understand why this would conflict with future API additions?

@benmccann
Copy link
Member

Maybe this line also needs to move to app.html?

https://github.com/sveltejs/site-kit/blob/cfb2931464b5d017c9536d2bb7a075ec721cb099/packages/site-kit/src/lib/components/ThemeToggle.svelte#L37

I'm having to recall, but I think the issue I was worried about is that if we added a handlePost API that the cookie concerns mentioned in #8676 (comment) might become an issue in that case

@Rich-Harris
Copy link
Member

closed by #11269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something isn't working p3-edge-case SvelteKit cannot be used in an uncommon way
Projects
None yet
Development

No branches or pull requests

8 participants