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

Keep reference equality when possible in update() #4968

Closed
wants to merge 2 commits into from

Conversation

davidmason
Copy link

For $set and $apply it is cheap and easy to check whether the new value will
be identical (===) to the old value and create a new object only when this is not
the case. This will reduce excessive renders when using the pureRenderMixin.

The same should be possible for for merge, but is more complicated so is not
attempted here.

Addresses #1923

@davidmason
Copy link
Author

I'm getting linting errors in a bunch of test files I haven't touched - is master failing these?

@davidmason
Copy link
Author

Would this change be better against 0.13-stable?

@sophiebits
Copy link
Collaborator

If you rebase, the lint errors should be fixed.

For $set and $apply it is cheap and easy to check whether the new value will
be identical (===) to the old value and create a new object only when this is not
the case. This will reduce excessive renders when using the pureRenderMixin.

The same should be possible for for merge, but is more complicated so is not
attempted here.
@davidmason
Copy link
Author

Thanks @spicyj, I think that fixed it.

@sebmarkbage
Copy link
Collaborator

FYI, this tool is deprecated in favor of immutable-js. I'm don't think we'll keep maintaining this repo. We can take this after we've cut our branch. However, if you want to keep using this tool I suggest you fork it and release your own npm package.

@sebmarkbage sebmarkbage added this to the 0.15 milestone Oct 6, 2015
@davidmason
Copy link
Author

if you want to keep using this tool I suggest you fork it and release your own npm package.

Might as well, can look at forking it next week.

@jimfb
Copy link
Contributor

jimfb commented Oct 15, 2015

@sebmarkbage Are we ready to merge this, or do we just want to deprecate it and let @davidmason take the code from here?

@rdquintas
Copy link

Hi, was wondering if I could help here.
Is this still an open issue ?

@jimfb
Copy link
Contributor

jimfb commented Dec 15, 2015

Ping @sebmarkbage. Are we ready to merge this, or do we just want to deprecate it and let @davidmason take the code from here?

@sophiebits
Copy link
Collaborator

@jimfb Feel free to merge this if it looks good to you.

@facebook-github-bot
Copy link

@davidmason updated the pull request.

@zpao zpao removed this from the 15.0 milestone Mar 4, 2016
@gaearon
Copy link
Collaborator

gaearon commented Mar 27, 2016

Thanks for contributing, @davidmason! I merged your PR locally but then tried to refactor it to avoid function allocations and the need to call getCurrentValue and getNextValue every time.

I submitted it in #6353 and kept all your commits attributed to you. I am keeping this PR open so when/if #6353 gets merged, this PR should appear merged as well (if I understand GitHub correctly 😅 ). Please check out #6353 and leave a note if I missed something. And thanks for getting this ball rolling!

@davidmason
Copy link
Author

Cool, thanks for keeping this going :)

@facebook-github-bot
Copy link

@davidmason updated the pull request.

@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2016

I’m going to close this for the same reasons as #6353 (comment).
I think this direction will be best for everyone involved.

@gaearon gaearon closed this Apr 1, 2016
@facebook-github-bot
Copy link

@davidmason updated the pull request.

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.

8 participants