Skip to content

fix cookie redirect matcher #261

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

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Conversation

khuezy
Copy link
Contributor

@khuezy khuezy commented Oct 7, 2023

When cookie['key-does-not-exist-and-will-be-undefined'], the cookie matcher logic would return true because undefined !== ""

See: cookies?.[redirect.key] !== ""

@changeset-bot
Copy link

changeset-bot bot commented Oct 7, 2023

⚠️ No Changeset found

Latest commit: 8936649

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented Oct 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
open-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 8, 2023 1:43pm

@@ -22,14 +22,14 @@ const routeHasMatcher =
switch (redirect.type) {
case "header":
return (
headers?.[redirect.key.toLowerCase()] !== "" &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@conico974 Should this be updated too?

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs to be updated as well, but i think we should do something like

headers?.[redirect.key.toLowerCase()] && headers?.[redirect.key.toLowerCase()] !== ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

!!headers?.[redirect.key.toLowerCase()] should take care of both those conditions right? undefined and empty string will both be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@conico974 gonna merge this unless I should make your suggested changes.

@yarinsa
Copy link

yarinsa commented Oct 10, 2023

#263 Should solve this issue

@yarinsa
Copy link

yarinsa commented Oct 10, 2023

What is the ETA for merging this?

@khuezy
Copy link
Contributor Author

khuezy commented Oct 10, 2023

There's 3 issues we'd like to get in for 2.2.2, not sure when that will get released, perhaps this week.

@khuezy khuezy merged commit 8483dea into opennextjs:main Oct 10, 2023
@yarinsa
Copy link

yarinsa commented Oct 10, 2023

Thank you

khuezy added a commit that referenced this pull request Oct 10, 2023
khuezy added a commit that referenced this pull request Oct 10, 2023
@yarinsa
Copy link

yarinsa commented Oct 19, 2023

@khuezy Why was it reverted?

@khuezy
Copy link
Contributor Author

khuezy commented Oct 19, 2023

@khuezy Why was it reverted?

If you click on the revert PR, there's a description there: #267
The issue was fix and merged into main but if you're still having issues then something's up.

The test app: https://github.com/sst/open-next/blob/main/examples/app-router/next.config.js
The e2e test: https://github.com/sst/open-next/blob/main/packages/tests-e2e/tests/appRouter/config.redirect.test.ts

13.5.6 test run: https://github.com/sst/open-next/actions/runs/6568405904/job/17842773477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants