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

Allow _redirects to be placed in root directory #1586

Merged
merged 2 commits into from
Jun 1, 2021

Conversation

roschaefer
Copy link
Contributor

This was easier to fix than expected. The fix should allow users to have their own _redirects file in the project root. If this file exists, it gets copied to build/ before adapter-netlify appends the catchall rule to it.

This behaviour is already documented, but currently broken.

How can I write an automated test for this adapter?

I did the following to test this manually:

  1. Use yarn link "@sveltejs/kit" and yarn link "@sveltejs/adapter-netlify" in my application repository to point the two node modules to my modified versions.
  2. Run svelte-kit build with a _redirects file in the project root.
  3. Check if content of build/_redirects is expected.
  4. Run svelte-kit build without a _redirects file in the project root.
  5. Check if content of build/_redirects is expected.

Any guidance or help how to match the desired code quality to get this PR merged is appreciated.

close #1585

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

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

This was easier to fix than expected. The fix should allow users to have their own `_redirects` file in the project root. If this file exists, it gets copied to `build/` before `adapter-netlify` appends the catchall rule to it.

This behaviour is [already documented](https://github.com/sveltejs/kit/tree/master/packages/adapter-netlify#using-netlify-redirect-rules), but [currently broken](sveltejs#1585).

How can I write an automated test for this adapter?

I did the following to test this manually:

1. Use `yarn link "@sveltejs/kit"` and `yarn link "@sveltejs/adapter-netlify"` in my application repository to point the two node modules to my modified versions.
2. Run `svelte-kit build` with a `_redirects` file in the project root.
3. Check if content of `build/_redirects` is expected.
4. Run `svelte-kit build` *without* a `_redirects` file in the project root.
5. Check if content of `build/_redirects` is expected.

Any guidance or help how to match the desired code quality to get this PR merged is appreciated.

close sveltejs#1585
@benmccann
Copy link
Member

@kvn-shn @sw-yx @mikenikles as our resident Netlify users, can you guys reproduce this and are you okay with this change?

@kvn-shn
Copy link
Contributor

kvn-shn commented May 29, 2021

I cannot reproduce this. Actually I wouldn't change it because I believe everything is working fine. There just seems to be a small misunderstanding, see #1585 (comment)

@swyxio
Copy link
Contributor

swyxio commented May 29, 2021

ah yeah what kvn-shn said. it needs to be in the final directory that gets served by netlify, and for sveltekit that is the static folder, not project root.

@roschaefer
Copy link
Contributor Author

Yes! 🎉 you're totally right. I'm going to close this as it's not needed.

@roschaefer roschaefer closed this May 29, 2021
@benmccann
Copy link
Member

I would have expected it to be in the root directory, so without having looked at this much, this didn't seem like a bad change to me. Do we think static is the better place for it? If so, should we add some documentation about that?

@swyxio
Copy link
Contributor

swyxio commented May 30, 2021

i do think its intuitive to be in the root for people who dont know how netlify works (and pretty self explanatory if you need to move the whole thing into a monorepo setup), but am hesitant to make a special rule that only works for sveltekit but not for anything else. i dont feel strongly about this one but we can always add some docs. did one #1596

@roschaefer
Copy link
Contributor Author

I'm totally happy putting my _redirects in static/ and I suggest to keep everything as it is. The only thing that could be improved is documentation to save others from the same confusion. I offered to write some when I closed the referenced issue #1585 (comment) but @sw-yx already did in #1596.

So all is well.

@benmccann benmccann reopened this Jun 1, 2021
@benmccann benmccann merged commit 3b988a4 into sveltejs:master Jun 1, 2021
@benmccann benmccann changed the title fix(adapter-netlify): Append to _redirects Allow _redirects to be placed in root directory Jun 1, 2021
@roschaefer
Copy link
Contributor Author

OK, I'm happy with my own solution, too! 🤷‍♂️ Proud to be a Svelte-Kit code contributor now.

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.

(adapter-netlify): _redirects file gets overwritten instead of changed
4 participants