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

Improve documentation for useSyncExternalStore #6530

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pwbriggs
Copy link

@pwbriggs pwbriggs commented Jan 4, 2024

Clarify the "I’m getting an error: “The result of getSnapshot should be cached” section of the useSyncExternalStore API reference documentation.

I put some notes about why the documented error occurs in a <DeepDive> section. It focuses on the details of comparison in JavaScript. I'm not sure if this is the intended use of deep dive sections (for example, how much prior knowledge about JavaScript do we assume the reader has? Who is the intended audience for the deep dive sections in the API reference? How well-known are the details of comparing objects in JavaScript?), but, in any case, I think this is at least an improvement.

Copy link

github-actions bot commented Jan 4, 2024

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Hey @pwbriggs, thanks for the suggestion and your help elsewhere on the docs! I left some comments for improving this even more, let me know what you think!

src/content/reference/react/useSyncExternalStore.md Outdated Show resolved Hide resolved
src/content/reference/react/useSyncExternalStore.md Outdated Show resolved Hide resolved
src/content/reference/react/useSyncExternalStore.md Outdated Show resolved Hide resolved
src/content/reference/react/useSyncExternalStore.md Outdated Show resolved Hide resolved
};

console.log(object1 == object2); // false
```
Copy link
Member

Choose a reason for hiding this comment

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

Something is missing here that connects it back to why return {} is a new object every time, something like this:


This also applies to the return values of functions:

function getUser() {
  return {
    name: "John",
    favoriteNumber: 42
  };
}

const object1 = getUser();
const object2 = getUser();

console.log(object1 == object2); // false

And maybe add an example of hosting the object out to show how it doesn't change?

Copy link
Member

Choose a reason for hiding this comment

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

The examples would be better as chat settings like in the explanations above rather than users.

Copy link
Author

Choose a reason for hiding this comment

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

@rickhanlonii I'm not sure how I should change the code example. I want it to be clear to the reader that this isn't specific to getSnapshot handlers (we include examples of that above), but it needs to include returning an object from a function (like you said).

We also want to make it clear that the values of the retuned object are the same, but then it doesn't make sense why you would be comparing the returned values.

The best I can think of is something like this:

function getMathConstants() {
  return {
    pi: 3.14159,
    e: 2.71828
  }
}

let mathConstants1 = getMathConstants();
let mathConstants2 = getMathConstants();

console.log(mathConstants1 == mathConstants2);
// false

But that makes very little sense. And, like you said, it should probably be related to the example chat app.

Any suggestions?

Copy link
Author

Choose a reason for hiding this comment

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

Update everything but the code example.
@rickhanlonii
Copy link
Member

Hey @pwbriggs, unrelated to this PR, I can't find a way to contact you. Can you email me or DM me on twitter? I appreciate all you do on the docs and I'd like to upgrade your permissions to be a maintainer if you're interested :)

@pwbriggs
Copy link
Author

Sure, thanks for the invitation! Do you have an email address you're willing to share? Do you have access to [email protected]'s inbox?

@rickhanlonii
Copy link
Member

Same as my GH handle at gmail

Copy link

vercel bot commented May 11, 2024

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

Name Status Preview Comments Updated (UTC)
19-react-dev ❌ Failed (Inspect) May 12, 2024 4:04am
react-dev ✅ Ready (Inspect) Visit Preview May 12, 2024 4:04am

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

Successfully merging this pull request may close these issues.

None yet

3 participants