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

fix: setting the right tag values for fetch cache #304

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

mathisobadia
Copy link
Contributor

Right now the tag property is always empty, I tried to check in the nextjs code base the reason and i found this relevant code
this code was introduced in this PR, So I'm wondering how we should manage that for older versions of nextjs that don't have that change.

Also ideally I would like to add some tests to the open-next repo to make sure that revalidateTag works ok but I still don't understand fully the setup and how to write that test so this PR is probably a WIP

Copy link

changeset-bot bot commented Nov 3, 2023

⚠️ No Changeset found

Latest commit: 4974457

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

Copy link

vercel bot commented Nov 3, 2023

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

Name Status Preview Comments Updated (UTC)
open-next ❌ Failed (Inspect) Nov 27, 2023 3:36pm

@mathisobadia
Copy link
Contributor Author

@khuezy I can confirm that this fixes the fetch cache issue in my app with latest nextjs and open-next version after testing it. So I think it can me merged. It also corresponds with what the new docs say are the parameters according to the latest PR https://github.com/vercel/next.js/blob/63ffdea4ab66a69d67d24161c995fd45e7b276f2/docs/02-app/01-building-your-application/08-deploying/index.mdx so we can hope that this will stay stable for a bit

@@ -298,7 +310,7 @@ export default class S3Cache {
// If we use an in house version of getDerivedTags in build we should use it here instead of next's one
const derivedTags: string[] =
data?.kind === "FETCH"
? data.data.tags ?? []
? ctx?.tags ?? []
Copy link
Collaborator

@khuezy khuezy Nov 27, 2023

Choose a reason for hiding this comment

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

Will this break backwards compatibility? Can we do ctx?.tags ??data?.data?.tags ?? []

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah you're right, that's how it should be done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated my PR with this change

@khuezy
Copy link
Collaborator

khuezy commented Nov 27, 2023

Thanks! The changes LGTM

@khuezy khuezy merged commit 250e9e1 into sst:main Nov 27, 2023
1 of 2 checks passed
@github-actions github-actions bot mentioned this pull request Nov 28, 2023
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.

None yet

3 participants