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

Issue#48 Optionally support object prototypes #51

Merged
merged 3 commits into from
Aug 8, 2015

Conversation

virajsanghvi
Copy link

Couple notes:

  • I'm still using Object.getPrototypeOf in instantiateEmptyObject because coming from any of the ImmutableObject methods, I wouldn't have options. An alternative, but not as clean, would be to dynamically generate instantiateEmptyObject in makeImmutableObject(), and then reattach it to results in the ImmutableObject methods (probably by passing some options arg to makeImmutableObject()). If you have a better idea, I'm all ears.
  • I spent some time trying to generate prototypes with jscheck, but I'm unsure how, and it was hard to find documentation/examples online. Let me know if there's anything you'd like me to do there as the tests are probably lacking, and pointers to docs would be helpful.

// Finalizes an object with immutable methods, freezes it, and returns it.
function makeImmutableObject(obj) {
addPropertyTo(obj, "merge", merge);
addPropertyTo(obj, "without", without);
addPropertyTo(obj, "asMutable", asMutableObject);
addPropertyTo(obj, "instantiateEmptyObject", instantiateEmptyObject);
Copy link
Owner

Choose a reason for hiding this comment

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

Let's inline instantiateEmptyObject into makeImmutableObject so we can just do the prototype check once instead of every time we instantiate an object:

var prototype = Object.getPrototypeOf(this);
var instantiateEmptyObject =
  (prototype === Object.prototype)
    ? instantiatePlainObject
    : (function() { return Object.create(prototype); });

// ... the other addPropertyTo calls go here ... //

addPropertyTo(obj, "instantiateEmptyObject", instantiateEmptyObject);

...and then, elsewhere in the file:

function instantiatePlainObject() { return {}; }

Copy link
Author

Choose a reason for hiding this comment

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

I want to point out that with the suggested approach, you'd still be doing the prototype check almost every time we instantiate an object. merge and without both call makeImmutableObject on their clones, and would go through this check. The added benefit over the previous approach is that asMutable and merges without anyChanges would not have to go through this check.

One option I was thinking about was passing an options hash to makeImmutableObject with instantiateEmptyObject as an option, which when present, would be used instead of constructing it.

Anyways, let me know if you still want to proceed with the above suggestions.

Copy link
Owner

Choose a reason for hiding this comment

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

+1 for passing it as an argument to makeImmutableObject! That sounds like the best approach yet.

@rtfeldman
Copy link
Owner

This looks great! Two things before merge:

  • I had one minor inline code suggestion
  • Let's document the prototype option in the README

Other than that, really excellent work! 🎉 Sorry I took so long getting to reviewing it.

@rtfeldman rtfeldman self-assigned this Aug 4, 2015
@virajsanghvi
Copy link
Author

Thanks for your help with adding this!

rtfeldman added a commit that referenced this pull request Aug 8, 2015
Issue#48 Optionally support object prototypes
@rtfeldman rtfeldman merged commit 7cafbde into rtfeldman:master Aug 8, 2015
@maartenvandillen
Copy link

I have a question about the use of the prototype param of Immutable. Is there any way I can use this when calling merge? I am trying to merge an object with prototype into my redux store but it loses the prototype after merging.

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

Successfully merging this pull request may close these issues.

3 participants