-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This 👏 looks 👏 so 👏 cool!
What's the impact of this on anyone who does not yet have access to Netlify's cookie-based redirects? Or has the feature been rolled out to everyone? |
the cookie-based redirects won't officially be rolled out for another several weeks, i believe. need to touch base with them on that. realistically, we could merge because netlify would just ignore the condition on the redirect and so the redirect would just be moot, but we'd be generating pages for nothing in the meantime. should probably just keep open until it's officially rolled out? |
9fbbaef
to
7f6a0eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!! It's coming together!
PS: I think it would be really good to have some Cypress tests for this! 😊
63177b1
to
162374a
Compare
162374a
to
403fbbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really really good aside from the major issue regarding optional catch-all routes at root-level, that we are discussing on Slack. We'll figure it out!! 💪
Test that prerendered pages and static files from /public are correctly served under the redirect engine used with cookie-based redirects when having an optional catch-all route at root-level.
We do not need `existsSync` in the `setupNetlifyFunctionForPage` helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!!
On the README, I think it's better not to flag "Preview mode" so prominently under Caveats. The lacking support for netlify dev is a really minor edge case. IMO, it's better mentioned at the end of the Preview locally
section:
Note:
netlify dev
does not yet fully support Next.js Preview Mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONGRATULATIONS! 🎉 ✨ YOU HAVE DONE IT! 👏 👏
PS: Keeping my fingers crossed on /* /:splat 200 🤞😁
still a few question marks to address with finn re: serving *.json data files for these pages, no-cache, etc and 3 redirect tests to modify