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

Add an original function #179

Merged
merged 3 commits into from
Sep 3, 2018
Merged

Conversation

RichieAHB
Copy link

@RichieAHB RichieAHB commented Aug 24, 2018

This addresses #153 and exposes an original function, useful for === comparing a proxy with an original object. The docs / tests should explain most of the implementation!

@coveralls
Copy link

coveralls commented Aug 24, 2018

Coverage Status

Coverage increased (+0.07%) to 93.212% when pulling c16b5b0 on RichieAHB:rahb/add-original into 2897e12 on mweststrate:master.

}

it("should return the original from the proxy", () => {
const nextState = produce(baseState, draftState => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you update the test to check both with useProxies(true) and useProxies(false)?

Copy link
Author

Choose a reason for hiding this comment

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

For sure 👍

})
})

it("should return the same object from an object that is not proxied", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add a test that for a newline introduced object on the draft it returns undefined?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the same for a value that is a primitive

Copy link
Author

@RichieAHB RichieAHB Aug 27, 2018

Choose a reason for hiding this comment

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

Ok, so you mean:

produce(baseState, draftState => {
    draftState.c = {};
    draftState.d = 3;
    expect(original(draftState.c)).toBeUndefined()
    expect(original(draftState.d)).toBeUndefined()
})

I wrote it such that it would just return the same object in that case as ultimately we just want the original object, and for those that think it may be being proxied when setting it it may be useful to not return a different type (i.e. undefined)? Happy to change this behaviour to undefined in the implementation though if you'd rather 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think returning undefined is slightly more intuitive / better supports fail-fast scenarios better. If you agree, I'll happily merge!

Copy link
Author

Choose a reason for hiding this comment

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

I've made those changes! It makes the types a bit fiddlier: anyone using TS / Flow with this function will have to check for undefined each time now as for TS I don't think you can pattern match on DraftObject due to that fact that any object extends it. And the Flow types don't try and make that distinction anyway.

@mweststrate
Copy link
Collaborator

Looks great! I added some comments about introducing a few more test cases

@mweststrate
Copy link
Collaborator

Merged! Thanks a lot

@mweststrate mweststrate merged commit f8479c2 into immerjs:master Sep 3, 2018
@RichieAHB RichieAHB deleted the rahb/add-original branch September 13, 2018 14:54
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.

4 participants