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

[breaking] remove createIndexFiles option, derive from trailingSlash instead #3801

Merged
merged 12 commits into from
Feb 10, 2022

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 9, 2022

createIndexFiles has been removed — it is now controlled by the trailingSlash option. See https://kit.svelte.dev/docs/configuration#trailingslash for more details

Original PR message:

Fixes #3799.

One thing I've noticed about this option is that people (including myself) get confused talking about it because the default is true, and it feels unusual to have to explicitly set an option to false. Two possible solutions:

  1. we come up with a better name for it that means the opposite, allowing a default of false
  2. we change the default to false, and make about/index.html instead of about.html opt-in

Might be worth doing a quick survey of popular static file hosts and seeing if about.html will work as expected in the majority of cases.

Also, it might make sense to vary the default based on the value of trailingSlash or the host being used, which we could do on a per-adapter basis, but not globally.

  • update docs

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Feb 9, 2022

🦋 Changeset detected

Latest commit: 1e2a61e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@sveltejs/adapter-static Patch
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 1e2a61e

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/6205694728e01d00087f8c8b

@benmccann
Copy link
Member

This seems like a good idea to me. It'd be nice to do it automatically for adapter-netlify (I guess based on the value of trailingSlash?) and some of the others

@Rich-Harris
Copy link
Member Author

Via https://www.zachleat.com/web/trailing-slash/, I found https://github.com/slorber/trailing-slash-guide. My takeaway is that we should indeed vary it based on trailingSlash (which effectively means defaulting to false).

I even wonder if it's worth exposing the option at all. I suspect there aren't really any cases where you'd need to control createIndexFiles independently of trailingSlash, and on the basis that APIs are easy to add and hard to remove, we should make the breaking change now and see if people complain.

@benmccann
Copy link
Member

I suspect there aren't really any cases where you'd need to control createIndexFiles independently of trailingSlash,

If you just drop the files in Apache or whatever you will need createIndexFiles: true. Does that mean users will also need trailingSlash: true in that case? That could be reasonable, but I feel like it'd need a screaming warning at the top of adapter-static or something because people wouldn't expect to need to look at the docs for that option most of the time and it could break a good handful of people.

@Rich-Harris
Copy link
Member Author

Is that true? If I create file.html and folder/index.html in the relevant directory and fire up Apache, this is what I get:

image

image

/file renders correctly, /file/ 404s. /folder redirects to /folder/ which renders correctly. Every webserver seems to have subtly different semantics, but AFAICT prettifying foo.html and bar/index.html to /foo and /bar/ respectively seems pretty consistent

@benmccann
Copy link
Member

Huh. TIL. I didn't think you could leave off the .html extension in the file case. Consider me corrected then

@benmccann benmccann changed the title move createIndexFiles option to adapter-static [breaking] remove createIndexFiles option, derive from trailingSlash instead Feb 10, 2022
@benmccann
Copy link
Member

lgtm after updating the validation message

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove prerender.createIndexFiles option in favour of adapter control
2 participants