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

Provide an optional parameter for inferring @action/@computed/@observable #219

Open
rrousselGit opened this issue Jul 10, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@rrousselGit
Copy link
Collaborator

This proposal is for automatically inferring @action/@computed/@observable based on the type of the variable.

The idea is, by letting the inference do its job, silly mistakes where we forgot to decorate a member can be stopped.

The inference would work using the following rules:

  • private/protected members, whatever their type is, don't benefit from inferred decorators.
  • public getters are inferred as @computed
  • public mutable property as inferred as @obvervable
  • public immutable properties aren't @observable
  • public methods are inferred as @action

This inference can be configured using two different ways:

  • using @Store decorator, for store specific override
    @Store(
      autoComputed: true,
      autoAction: true,
      autoObservable: true,
    )
    class MyStore { ... } 
  • inside the build.yaml file, for global configuration change:
    targets:
      $default:
        builders:
          mobx_codegen:
            options:
              autoComputed: true
              autoAction: true
              autoObservable: true

I've looked through the entire mobx_examples folder and all of the examples follows these rules.

There are some exceptions, like in form_store.dart where setupValidations is not decorated.
But setupValidations should probably be performed inside the constructor of FormStore and therefore setupValidations can be made private.

Another exception is in github_store.dart, where repositories is not @obvervable.
But that seems dangerous considering repositories is mutated on multiple occasions and is used inside the build method of RepositoryListView.

@eltonomicon
Copy link

I like the @store attribute with optional parameters to enable inference. Comments:

  • If you want to infer, I think the high % case would be to infer all 3. I think it would be best to have an option to infer all 3 with 1 parameter instead of having to always specify 3 parameters.
  • 'public getters are inferred as @computed' seems to conflict with 'public immutable properties aren't @observable'. Also IMO there is a case for observable readonly properties, see Support for read-only observables #220, although maybe this isn't an inference use case.

Would inference be all-or-nothing or could you still use annotations to override inference rules?

@pavanpodila
Copy link
Member

Great idea @rrousselGit. Overall I like the direction. Few other things to consider:

  • The defaults should be false I think, in order to avoid an collisions with the original intents of the Store class. Not all getter functions are computed (as @eltonomicon pointed out), not all props are observables by default.
  • There should be a way to turn off the auto-generation for some props (eg @noAction, @noComputed, @noObservable). Essentially a per-member toggle that can be used to opt out of the auto generation.

@smiLLe
Copy link

smiLLe commented Jul 11, 2019

@pavanpodila you often see ignore: true . so for computed it would be @computed(ignore: true)

@eltonomicon
Copy link

Agreed with ignore: true, IMO this is preferable to doubling the number of annotations.

@pavanpodila
Copy link
Member

Ya like the ignore approach.

@rrousselGit
Copy link
Collaborator Author

It should be @Computed() in that case.

Anyway I agree with ignore. If peoples wants a shorthand to make it easier to use, they can do the following:

const noComputed = Computed(ignore: true);

@shyndman shyndman added the enhancement New feature or request label Dec 20, 2019
@pavanpodila
Copy link
Member

@shyndman I was thinking we can pick this up as a good enhancement for simplifying the use of MobX for most users. The current @store directive can get a nitro-boost with this auto-inference feature? What do you think ?

@shyndman
Copy link
Collaborator

shyndman commented Dec 26, 2019

Yeah, I'm into it, along with the opt-out annotation.

Maybe we could generalize the opt-outs into a single annotation? Something like @ignore()?

@pavanpodila
Copy link
Member

pavanpodila commented Dec 27, 2019 via email

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

No branches or pull requests

5 participants