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

Updated Immutable Data Stuctures Docs #9845

Merged
merged 4 commits into from
Jul 6, 2017
Merged

Conversation

kastentx
Copy link
Contributor

@kastentx kastentx commented Jun 4, 2017

I corrected the issue I raised in issue #9844 in the Optimizing Performance > Using Immutable Data Structures of the documentation.

I also just submitted a CLA. Thanks!

@@ -383,7 +383,7 @@ const y = x.set('foo', 'baz');
x === y; // false
```

In this case, since a new reference is returned when mutating `x`, we can safely assume that `x` has changed.
In this case, since a new reference is returned when mutating `x`, we can safely assume that `x` has not changed.
Copy link
Contributor

@aweary aweary Jun 5, 2017

Choose a reason for hiding this comment

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

I see where the confusion is coming from, but I think this is also missing the point that's it's trying to communicate. When it says

we can safely assume that x has changed.

It means we know the value that x was holding, which was updated and stored in a new reference y, has changed because the reference equality check returned false.

We should clarify this point better. I suggest something like:

In this case, since a new reference is returned when mutating x we can use a reference equality check (x === y) to verify that the new value stored in y is different than the original value stored in x.

It might also be useful to demonstrate that updating a record with the same value results in a passing equality check:

const y = x.set('foo', 'bar')
x === y // true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reply! I've implemented these changes in a new commit.

@kastentx
Copy link
Contributor Author

kastentx commented Jun 7, 2017

Great points! I think what we've got now is alot more clear.

@aweary
Copy link
Contributor

aweary commented Jul 6, 2017

Looks great, thanks @kastentx!

@aweary aweary merged commit 8f4d307 into facebook:master Jul 6, 2017
gaearon pushed a commit that referenced this pull request Jul 11, 2017
* updated immutable data structures section

* improved immutable clarifications

* changes to example immutable code

(cherry picked from commit 8f4d307)
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

4 participants