-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(guides/single-fetch): add some recommended steps #9534
Conversation
|
Hi @michenly, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 馃コ |
Thank you @michenly! |
docs/guides/single-fetch.md
Outdated
|
||
Please also read through the [Breaking Changes][breaking-changes] before starting so you can be aware of some of the underlying behavior changes, specifically around serialization and status/header behavior. | ||
|
||
## Breaking Changes |
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.
I worry putting this section first will give the wrong impression - that folks need to make all these changes before they can enable single fetch because the order in which they'll read the doc is Breaking Changes -> Enabling
.
Technically, I think the only thing they need to do to start using it is the fetch
polyfill update. the rest of the "breaking changes" should be almost 100% backwards compatible and able to fixed incrementally after enabling the feature.
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.
Yeah, that's a good point, I agree. It also follows the flow in the overview
Remix introduced support for "Single Fetch"...
Enabling Single Fetch is incredibly simple...
Please also read through the Breaking Changes...
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.
Technically, I think the only thing they need to do to start using it is the fetch polyfill update. the rest of the "breaking changes" should be almost 100% backwards compatible and able to fixed incrementally after enabling the feature.
I agree, it's more about awareness of how it's effecting your code. The "Deprecated headers export" might be the only exception. You're right, it's not breaking, but if people are heavily using a CDN it's good to know how Single Fetch effects that piece.
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.
ah yeah good call - headers
also needs to be updated to adopt
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.
Maybe we can more clearly delineate the breaking changes as "up front" and "iteratively after the fact"
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.
If there is a concern I am 馃憤 with moving the block down. I originally move it up because the overview made a point of reading through underline changes and it had made sense to me that folk should understand what had really changed under the hood before using it.
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.
I can take a stab at this sometime today
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.
OK, I moved the breaking changes section back below and separated them by "up front" and "over time". Let me know if there's any last feedback and I'll get this merged today! Thanks!
Some recommended docs changes.
Feel free to edit on top of if as you see fix 馃檹