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!: make useCdn use true by default #191

Merged
merged 1 commit into from
Apr 27, 2023
Merged

fix!: make useCdn use true by default #191

merged 1 commit into from
Apr 27, 2023

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Apr 4, 2023

Why true is better

Seeing unexpected stale content is a far better problem to have than a surprise overage bill 😅
This is a follow up to this discussion.

Also worth noting that one of the historical reasons why useCdn: false have been the default were the limitation the client had prior to v3: https://www.sanity.io/help/js-client-cdn-configuration#:~:text=subscribing%20to%20changes).-,Gotcha,-Prior%20to%20v3.0.0
Now that the client have supported using token with useCdn: true for quite some time it makes sense to change the default.

This will be a breaking release

The release note will highlight this as:
BREAKING CHANGE: if you want to keep the previous behavior, add useCdn: false to your configuration.

@stipsan stipsan requested a review from rexxars April 4, 2023 18:46
@kmelve
Copy link
Member

kmelve commented Apr 4, 2023

This is a neat way of preventing some of the unintended cases of hitting the API limits.

The semver change seems a bit tricky, but then again, everyone who have been using this with undefined should have seen the warning. It seems unlikely that someone are relying on the undefined behavior for business critical functionality.

That being said, what's unclear to me is how we can make sure that the developer experience for folks that use the client to fetch content for static rendering isn't getting more confusing, as well as typical onboarding scenarios, and local development feedback loops.

New users and onboarding

How will this affect developers new to the platform who are asked to publish a document and see their change in the frontend? Maybe the cache invalidation is quick enough that it's fine? Either way, we should make sure to make a note of this property in new onboarding materials.

Documentation

This release could incentivize writing better docs on how our caching actually works, including invalidation. I see the "why aren't I'm seeing my published content" quite often.

It would also be very helpful if we provided guidance on how to set this property so it works with typical static rendering where builds or frontend revalidation is triggered by a webhook. This also affect content creators who are confused by not seeing their published content. I get that it's slightly outside of this change specifically, but it does bring it into focus.

@stipsan
Copy link
Member Author

stipsan commented Apr 5, 2023

To solve the general problem of stale content we could, when publishing a document, show an intermediary "yellow" state: Draft -> Pending/Propagating/Publishing -> Published.
This state can be disabled in the studio config, it's enabled by default. A mouseover tells you if it's fully available in the API CDN yet or how long until it is (we have metrics on the timing of this).

For the static site building I think it needs better docs indeed that is tailored specifically for that. There's a lot of moving parts, ISR or no, webhooks or no, etc.

README.md Outdated Show resolved Hide resolved
runeb
runeb previously requested changes Apr 11, 2023
README.md Outdated Show resolved Hide resolved
src/warnings.ts Outdated Show resolved Hide resolved
test/warnings.test.ts Outdated Show resolved Hide resolved
@runeb
Copy link
Member

runeb commented Apr 11, 2023

Added suggested changes from @tarungangwani

Copy link
Member

@rexxars rexxars left a comment

Choose a reason for hiding this comment

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

While the change in and of itself looks fine, I don't agree that this shouldn't be considered a breaking change. People may have just ignored the warnings on app startup, for instance. I'd vote to bump the major version.

BREAKING CHANGE: if you  want to keep the previous behavior, add `useCdn: false` to your configuration.

Co-Authored-By: Rune Botten <rbotten@gmail.com>
@rexxars rexxars changed the title fix: make useCdn use true by default fix!: make useCdn use true by default Apr 12, 2023
@stipsan stipsan removed the request for review from runeb April 12, 2023 19:32
@stipsan stipsan merged commit 601759c into main Apr 27, 2023
@stipsan stipsan deleted the use-cdn-by-default branch April 27, 2023 21:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants