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

Support prerender in Netlify redirects #5904

Merged
merged 3 commits into from
Jan 23, 2023
Merged

Conversation

matthewp
Copy link
Contributor

Changes

  • Fixes [Prerender] Pages with dynamic routes in the same folder don't work in @2.0.0-beta.2 #5860
  • A little bit of a refactor of the logic that generates the _redirects file to be a little more structured.
  • Rather than use the build-generated "distURL", which is always going to be wrong for dynamic routes, builds it itself and uses Netlify's placeholder syntax. For spread routes it uses the * syntax.
  • Prettifys it a bit so that each part, the input, target, and status all line up.

Testing

  • Test added for the same scenario that the bug describes.

Docs

N/A, bug fix.

@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2023

🦋 Changeset detected

Latest commit: 1fdc937

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

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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jan 19, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what needs to be fixed now. I tested this locally, I wonder if the functions/fixtures/404/dist/_redirects is correct now though. It is:

/*      /.netlify/functions/entry    404
/       /.netlify/functions/entry    200
/404    /.netlify/functions/entry    200

where the /* is the first and I think that causes a 404 always? Before this PR it's:


  /    /.netlify/functions/entry    200
  /404    /.netlify/functions/entry    200
  /*    /.netlify/functions/entry    404

I think we need to sort to like before for this case.

@matthewp
Copy link
Contributor Author

Ah yeah, you're right. I think I was actually wrong about needing to sort dynamic routes first. What you really want is to sort any exact route first. It's the wildcard/spread routes that need to come last. I think I'll add a specificity integer and sort that way. I'll add a test for this too.

Comment on lines +29 to +30
expect(redir).to.include('/pets/:cat /pets/:cat/index.html 200');
expect(redir).to.include('/pets/:dog /pets/:dog/index.html 200');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these conflict? I expect that they might and only /pets/:cat would be matched.

Since all of these are generating actual files we might need to track any files that the dynamic route has generated, then add redirects for the exact route?

expect(redir).to.include('/pets/cat1       /pets/cat1/index.html        200');
expect(redir).to.include('/pets/cat2       /pets/cat2/index.html        200');
expect(redir).to.include('/pets/cat3       /pets/cat3/index.html        200');
expect(redir).to.include('/pets/dog1       /pets/dog1/index.html        200');
expect(redir).to.include('/pets/dog2       /pets/dog2/index.html        200');
expect(redir).to.include('/pets/dog3       /pets/dog3/index.html        200');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do conflict in an SSR context but in SSG since they are pre-generated they are just redundant. Meaning Netlify is going to use the first one and ignore the second; but that doesn't matter because it will go to the right file anyways.

The user should probably combine these into 1 route, but we don't require that at the moment so this is a valid (if odd) way to split up different "types" of data in SSG.

@matthewp
Copy link
Contributor Author

Ok, I think I got it now @bluwy here: 00224f3

The changes are:

  • Astro already sorts the route array so we don't need to resort what is produced for _redirects.
  • But, the wildcard 404 route is the exception. Since this is a new route that we create we need to sort it. So this change gives it a lower weight so its always sorted last. This is what you want, any defined routes are hit and then there's a final wildcard 404.

packages/integrations/netlify/src/shared.ts Outdated Show resolved Hide resolved
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@matthewp matthewp merged commit f5adbd6 into main Jan 23, 2023
@matthewp matthewp deleted the netlify-prerender-redirects branch January 23, 2023 14:47
matthewp added a commit that referenced this pull request Jan 26, 2023
* Support prerender in Netlify redirects

* Updated sorting algorithm

* Update packages/integrations/netlify/src/shared.ts

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Prerender] Pages with dynamic routes in the same folder don't work in @2.0.0-beta.2
3 participants