Ability to bail out of autoAction

[jeffijoe]

I am trying to upgrade mobx-task to support MobX v6, however, I am running into an issue due to observable and friends wrapping the Task function in an action, losing the task state.

The following test fails due to the .pending property being undefined.

https://github.com/jeffijoe/mobx-task/blob/24704082d91723e51bc8b426d3181e0ac5e02217/src/__tests__/task.test.ts#L235-L246

test('can be tacked onto an observable', async () => {
  const store = observable({
    todos: [],
    fetchTodos: task(async () => {
      return [{ text: 'install mobx-task' }]
    })
  })

  expect(store.fetchTodos.pending).toBe(true) // this returns undefined in v6
  await store.fetchTodos()
  expect(store.fetchTodos.resolved).toBe(true)
})

Would it be possible to add a feature that allows you to bail out of the auto-conversion?

Example:

// Could import a symbol
import { MOBX_BAIL_OUT } from 'mobx'

interface Task<T> {
  (): Promise<T>
  readonly pending: boolean
  readonly resolved: boolean
  match<R>(...): R
}

// Disregard input types..
function task(inner: Function): Task {
  function wrapped(...args) { /* ...*/ }
  wrapped[MOBX_BAIL_OUT] = true
  wrapped.pending = true // object defineprop omitted for brevity
  return wrapped
}

const store = observable({
  // The presence of the symbol on the `task()` result would
  // prevent MobX from wrapping it
  fetchTodos: task(async () => await fetch(...))
})

store.fetchTodos.match({..})

This would also apply to decorators.

Without this, I am unsure how to upgrade the library to support v6.

[mweststrate]

wrapping the Task function in an action, losing the task state.

Could you clarify on this part. Why does turning the function into an action break that? Does the context resolve wrongly or something like that.

What, ugly, potentially work is marking the function already as being an action, or prewrapping it as action. E.g. we do keep a flag about things being actions, and we are / should use that to avoid double wrapping: https://github.com/mobxjs/mobx/blob/main/packages/mobx/src/core/action.ts#L51

[jeffijoe]

@urugator yeah I could wrap it in an action, but since tasks are async, we usually wouldn't be modifying state in there, however some people use action.bound for the free binding, and some people will modify state before the first await which usually works, however I always recommend people not to rely on that.

My primary interest is to prevent the auto-conversion, which isMobxAction accomplishes, but since it's sort of an internal detail, I was wondering if a more official way like proposed would make sense.

[urugator]

and some people will modify state before the first await which usually works, however I always recommend people not to rely on that.

I don't understand what's the actual concern here, like how does the presence of action matters, what actually changes for the user, behavior wise?

some people use action.bound for the free binding

Explicit annotations/decorators don't rely on autoconversion and I think they will create new action even from existing, not sure.

[jeffijoe]

what actually changes for the user, behavior wise?

When using mobx.configure({ enforceActions: 'always' }), it would be weird if that wasn't enforced for task.

Behavior aside, would there be any performance implications with action? A task isn't supposed to be used as an action in the Mobx-sense. I am fine with leaving the isMobxAction on there, I just hope it's considered public API and that if you change it down the road it'll be considered a breaking change. :)

[urugator]

I see, got kinda confused by the https://github.com/jeffijoe/mobx-task#what-is-it example. I thought it's about getting rid of those runInAction calls, while it's actually about that extra state and user is still required to provide action when mutating his own state as demonstrated in other examples.

performance implications with action?

negligible

just hope it's considered public API

I don't think it is currently.

leaving the isMobxAction on there,

I think it's equally weird to claim something is action, while it isn't... imagine user checking for isAction(task()), would report false positive... Not sure how to go about this.

[jeffijoe]

I think it's equally weird to claim something is action, while it isn't.

That's fair. I tried it with action on the inside and it works. I appreciate the input fellas! With that said, I still believe the proposed feature would be useful for others building their own primitives on top of MobX. 😄