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

Idea: non-callback form of immer #302

Closed
mweststrate opened this issue Jan 25, 2019 · 18 comments
Closed

Idea: non-callback form of immer #302

mweststrate opened this issue Jan 25, 2019 · 18 comments

Comments

@mweststrate
Copy link
Collaborator

Occasionally I have cases where the callback based api of Immer is to limited, for example when I don't have explicit control over the flow or timing that drafts should be modified. For example in asynchronous processes where the draft should be updated bit by bit. In those cases I would like an api in which I can have more control, for example:

import { createDraft, finishDraft } from "immer"

const todo = { title: '', done: false }

async function loadTodo() {
    const draftTodo = createDraft(todo)
    draftTodo.title = await fetchTitle()
    draftTodo.done = await fetchDone()
    todo = finishDraft(draftTodo)
}
@Yurickh
Copy link

Yurickh commented Jan 26, 2019

I don't know much about immer internals, but could a similar result be achieved by allowing the user to pass a function that returns a promise to immer? Something along the lines of:

import produce from 'immer'

const todo = { title: '', done: false }

function loadTodo() {
  return produce(todo, async draft => {
    draft.title = await fetchTitle()
    draft.done = await fetchDone()
  })
}

@jamiewinder
Copy link

Just one idea off the back of this. Would it / might it be possible to leave the draft 'active' so you can produce multiple variants of it? For example:

const draft = createDraft(todo);
draft.title = 'Updated title';
const todoWithUpdatedTitle = finishDraft(draft);
draft.done = true;
const todoWithUpdatedTitleAndDone = finishDraft(draft);

@pantoninho
Copy link

I think this is a really good feature.

Yurickh's suggested API looks pretty straightforward and makes a lot of sense (when I first used Immer, I thought it would support that exact call).

@benjaminketron
Copy link

It would meet our needs as we would like to be able to pass a draft into asynchronous workflow logic and then finalize the draft upon workflow completion.

@aleclarson
Copy link
Member

aleclarson commented Jan 30, 2019

Note that @Yurickh's proposal would be a breaking change.

edit: Nevermind, I misunderstood!

edit 2: Actually, this will be a breaking change. See here: #307 (comment)

@Yurickh
Copy link

Yurickh commented Jan 30, 2019

Would it, really? I mean, it looks like an extension of the current API, rather than breaking.

@aleclarson
Copy link
Member

aleclarson commented Jan 30, 2019

@Yurickh The current behavior returns the Promise as a replacement of the base state. We can't assume no one is relying on that.

@Yurickh
Copy link

Yurickh commented Jan 30, 2019

I may have expressed myself poorly, but that was kind of how I was thinking it would go.
produce would return the promise that would resolve to the final value.
Usage would go like:

const newTodo = await produce(todo, async draft => { /* manipulation */ })

@aleclarson
Copy link
Member

aleclarson commented Jan 30, 2019

Oh, that's a great idea! We might be able to support that and the OP in one fell swoop, since they both defer the finalize phase. If not, I would implement your idea first.

I'm working on a PR for this! 👍

@jamiewinder
Copy link

If produce returns a Promise then all functions that use it need to be async in order to await it (or handle promises), right? Maybe I'm misunderstanding.

I think the async example in the original post is only one of many possible use cases. The non-callback form that @mweststrate suggested is the most basic form that immer could take, and can easily be used to create produce and produceAsync variants.

aleclarson added a commit to aleclarson/immer that referenced this issue Jan 30, 2019
Drafts won't be revoked until the returned promise is fulfilled or rejected.

As suggested by @Yurickh in immerjs#302
@pantoninho
Copy link

pantoninho commented Jan 31, 2019

I really like @Yurickh's suggestion, and I think it could be implemented without a breaking change:

// if recipe returns a promise, produce returns a promise too
const nextState = await produce(state, async (draft) => draft);

// if recipe does not return a promise, then produce 
// is synchronous and returns the value right away.
// (today's behaviour -> no breaking change)
const nextState = produce(state, draft => draft);

@aleclarson
Copy link
Member

@pantoninho It's already implemented in #305 😉

@pantoninho
Copy link

That was fast!
Thank you, this will be very useful for me.

Great work @aleclarson and everyone involved.

aleclarson added a commit to aleclarson/immer that referenced this issue Jan 31, 2019
Drafts won't be revoked until the returned promise is fulfilled or rejected.

As suggested by @Yurickh in immerjs#302
aleclarson added a commit to aleclarson/immer that referenced this issue Jan 31, 2019
Drafts won't be revoked until the returned promise is fulfilled or rejected.

As suggested by @Yurickh in immerjs#302
@mweststrate mweststrate mentioned this issue Feb 2, 2019
4 tasks
aleclarson pushed a commit that referenced this issue Feb 2, 2019
@aleclarson
Copy link
Member

Looks like async/await support will be a breaking change after all. See here: #307 (comment)

@aleclarson
Copy link
Member

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@scriby
Copy link
Contributor

scriby commented Feb 3, 2019

I'm curious about what happens when there are multiple async draft edits going at once. It seems like after one of them has gone async the state may be modified before it picks up again, which means it's now working against an older version of state. It also seems like there's some potential for conflicts due to the async sequencing.

Am I misunderstanding, or is there a pretty good source of potential danger here if you don't understand what's going on under the covers pretty well?

@mweststrate
Copy link
Collaborator Author

mweststrate commented Feb 3, 2019 via email

@scriby
Copy link
Contributor

scriby commented Feb 3, 2019

I think you're right about the base library, I was thinking about this in the context of something I'm trying to do with immer. I'll open a new issue to discuss instead of derailing this one too much more. Thanks!

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

No branches or pull requests

7 participants