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

feat: add Immer class #258

Merged
merged 0 commits into from
Dec 15, 2018
Merged

feat: add Immer class #258

merged 0 commits into from
Dec 15, 2018

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Nov 30, 2018

Closes #254

Every instance of the Immer class provides a bound produce function. As explained in #254, this class makes room for advanced configuration (if we need it) and allows for interop between Immer-using libraries that have different needs.

I took the time to refactor while I was implementing this, so the diff may look
more overwhelming than it is. :)

Summary

  1. Renamed immer.js to index.js so the Immer class can have immer.js

  2. Use the word "draft" in place of "proxy" in most places, because "draft" is
    a higher level of abstraction. It also makes sense to use the same semantics
    both internally and externally.

  3. Moved finalize logic to the Immer class

  4. Inlined the freeze function as only one callsite existed

  5. Inlined the createState functions in both proxy.js and es5.js

  6. Extract repeated code from produceEs5 and produceProxy (which have since
    been removed) into the Immer class

  7. The es5.js and proxy.js modules now have different exports:

    • scopes: an array of nested produce calls, where each value is an array
      of unrevoked proxies
    • currentScope(): shortcut for getting the last value of the scopes array
    • createDraft(): a renamed createProxy with the arguments swapped
    • willFinalize(): called right after the recipe function returns (only
      used by ES5 proxies)
  8. Changed some "draft state" properties:

    • removed hasCopy in ES5 state (checking for truthiness of copy does the job)
    • renamed proxy to draft in ES5 state
    • renamed finished to revoked in ES5 state
    • renamed proxies to drafts in Proxy state
    • added revoke method (called by the Immer class)
    • added draft property to Proxy state
    • use null literals instead of undefined (for brevity)
  9. Delay creation of patches and inversePatches arrays until the recipe
    function returns. This avoids array allocations when a rollback is performed
    by throwing.

  10. Simplified generatePatches by removing the last two arguments, since they
    can be obtained from the state argument.

@mweststrate
Copy link
Collaborator

mweststrate commented Nov 30, 2018 via email

@mweststrate
Copy link
Collaborator

mweststrate commented Nov 30, 2018 via email

@aleclarson
Copy link
Member Author

aleclarson commented Nov 30, 2018

@mweststrate Looks like the UMD bundle went from 8.88KB (3.29KB minified) to 9.8KB (3.56KB minified). Disappointing; I thought this would reduce the size. :(

edit: Probably from inflating the class declaration.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 96.931% when pulling c1a548b on aleclarson:factory into d157d7a on mweststrate:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 96.931% when pulling c1a548b on aleclarson:factory into d157d7a on mweststrate:master.

@coveralls
Copy link

coveralls commented Nov 30, 2018

Coverage Status

Coverage decreased (-0.3%) to 97.293% when pulling 1caaebd on aleclarson:factory into 9345f13 on mweststrate:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 96.931% when pulling c1a548b on aleclarson:factory into d157d7a on mweststrate:master.

@aleclarson
Copy link
Member Author

Tests passing! :)

I wonder if the Coveralls bot can be fixed?

@aleclarson
Copy link
Member Author

aleclarson commented Nov 30, 2018

I separated the diff noise out, so you can use 1fc76a7 for easier reviewing.

src/patches.js Outdated Show resolved Hide resolved
@aleclarson
Copy link
Member Author

Updated the .d.ts but not the .js.flow since I'm not using Flow currently.

Copy link
Collaborator

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Great PR @aleclarson !

Made some minor comments, but beyond that all LGTM. Thanks a lot!

src/immer.js Outdated Show resolved Hide resolved
src/patches.js Outdated Show resolved Hide resolved
src/es5.js Outdated Show resolved Hide resolved
@aleclarson
Copy link
Member Author

🎉 This PR is included in version 1.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants