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

MobX 7 roadmap #3796

Open
17 tasks
mweststrate opened this issue Nov 14, 2023 · 16 comments
Open
17 tasks

MobX 7 roadmap #3796

mweststrate opened this issue Nov 14, 2023 · 16 comments

Comments

@mweststrate
Copy link
Member

mweststrate commented Nov 14, 2023

[WIP for now, needed the issue number]

MobX 8 or later

@pzmosquito
Copy link

awesome news! It looks like the new version will be focused on removing deprecated stuff and updating the build system instead of introducing new features or breaking changes. It should be a breeze to upgrade!

@kubk
Copy link
Collaborator

kubk commented Nov 15, 2023

I have a somewhat controversial suggestion: automatically bind class methods in makeAutoObservable/makeObservable. The idea is to set { autoBind: true } as the default, while still offering the option to disable it with { autoBind: false }.

Rationale: People might forget to bind this. Here are a few examples:

Developers these days don't usually deal with .bind, .call, or .apply, as was common before. Hence, the cause of the issue isn't immediately clear to them. They may experience loss of reactivity and attribute it to Mobx.

While there might be reasons to preserve the original this by default, I fail to see them. All of my Mobx stores use { autoBind: true }. Also, though I'm not a Vue expert, it seems they bind this as well: Vue.js Computed Properties.

  computed: {
    // a computed getter
    reversedMessage: function () {
      // `this` points to the vm instance
      return this.message.split('').reverse().join('')
    }
  }

Perhaps we could even consider making new @action / @flow decorators bound by default, as well as introducing @action.unbound / @flow.unbound. I can work on PR for this.

@kiejo
Copy link

kiejo commented Nov 17, 2023

As a long-time user of MobX I would be in favor of keeping support for mobx-react / observer classes as it's still fully supported by React. I understand why new code bases might not use class based components, but for existing code bases it doesn't necessarily make sense to migrate all existing code if it still works well.

@mweststrate
Copy link
Member Author

I have a somewhat controversial suggestion: automatically bind class methods in makeAutoObservable/makeObservable. The idea is to set { autoBind: true } as the default, while still offering the option to disable it with { autoBind: false }.
Rationale: People might forget to bind this. Here are a few examples:

Interesting! The rationale for not doing so was that autobinding is expensive; it creates a function closure per instance rather than per type. MST does autobinding and I regretted that, due to the mem / cpu consumption. So I'm a bit torn on this one, being able to opt out can avoid the MST problem. On the other hand, if @action doesn't bind, while makeObservable does, that might be confusing. Maybe just makeAutoObservable, which is than an alias for "the most inefficient but convenient way to make observables"?

@mweststrate
Copy link
Member Author

As a long-time user of MobX I would be in favor of keeping support for mobx-react / observer classes as it's still fully supported by React. I understand why new code bases might not use class based components, but for existing code bases it doesn't necessarily make sense to migrate all existing code if it still works well.

Without provider / inject, and non-reactive props/state like in the latest version, mobx-react observer has actually become a really thin wrapper around mobx-react-lite's observer, so it might be worth just merging them again.

@kiejo
Copy link

kiejo commented Nov 17, 2023

Without provider / inject, and non-reactive props/state like in the latest version, mobx-react observer has actually become a really thin wrapper around mobx-react-lite's observer, so it might be worth just merging them again.

That sounds great!

@vincentLiuxiang
Copy link

Remove mobx-react / observer class support?

Hi @mweststrate can we keep observer for class?

Even though functional/hooks React components are very popular in nowdays, Oop programming is still very highly efficient for some UI interaction complex scenarios.

Eg:

When 2 pages are 90% similar, we can finish Page A. Page B can extend Page A and share 90% of the code, only overriding 10%.

The code between PageA and PageB is completely isolated and not coupled. so the code is very clean, which is conducive to later maintenance.

@observer 
class PageA extends React.component {
  renderSharedSection() {...}
  renderDiffSection() {...}
  render() {
     return (
       <>
         {this.renderSharedSection()}
         {this.renderDiffSection()}
       </>
     );
  }
}

class PageB extends PageA {
   renderDiffSection() {...}
}

Currently, we are in charge of more than 20+ products, web-IDE, data visualization etc...
This feature really helps us to improve our code quality and dev efficiency.

hope you can keep observer for class in the V7 version

(Since v6 doesn't support @ decorator, we are very excited to know V7 is coming 😂)

@mweststrate
Copy link
Member Author

mweststrate commented Nov 17, 2023

@vincentLiuxiang see also the comments just before yours :). Rephrased in the proposal. P.s. v6 does support @ decorator now: https://michel.codes/blogs/mobx-decorators

@vincentLiuxiang
Copy link

@vincentLiuxiang see also the comments just before yours :). Rephrased in the proposal. P.s. v6 does support @ decorator now: https://michel.codes/blogs/mobx-decorators

👍👍👍
@mweststrate

would like to confirm whether there is any API incompatibility when upgrading from 6.10.0 to 6.11.0 ?

@mweststrate
Copy link
Member Author

not by design (unless you switch from legacy to modern decorators)

@terrasoff
Copy link

Considering the massive feature removal, sounds like the bundle size will be significantly reduced 👍

@emereum
Copy link
Contributor

emereum commented Dec 5, 2023

Can there be a clear upgrade path for large codebases? For example if there are breaking changes can codemods be provided? Or alternatively, can the breaking changes be made in such a way that consumers could write their own codemods?

If you'd like feedback on what breaks / what works in large codebases at certain points through this roadmap I would be happy to contribute.

@Bnaya
Copy link
Member

Bnaya commented Jan 12, 2024

It's worth pinning this issue <3

@peter-cardenas-ai
Copy link

@mweststrate You're referring to intercept and observe here, not the observer HOC correct?

@xaviergonz
Copy link
Contributor

is there an alternative to intercept and observe? mobx-keystone relies on those heavilty

@MirKml
Copy link

MirKml commented Jun 6, 2024

I have a somewhat controversial suggestion: automatically bind class methods in makeAutoObservable/makeObservable. The idea is to set { autoBind: true } as the default, while still offering the option to disable it with { autoBind: false }.

Interesting! The rationale for not doing so was that autobinding is expensive; it creates a function closure per instance rather than per type. MST does autobinding and I regretted that, due to the mem / cpu consumption. So I'm a bit torn on this one, being able to opt out can avoid the MST problem. On the other hand, if @action doesn't bind, while makeObservable does, that might be confusing. Maybe just makeAutoObservable, which is than an alias for "the most inefficient but convenient way to make observables"?

I'm against default { autobind: true }, we had many stores makeAutoObservable with autobind : true, but we clear them all.
Reason - when we applied eslint, we realized that autobinded methods are unwanted magic.

store class

export class Store {
    constructor() {
        makeAutoObservable(this, null, { autoBind: true });
    }
    
    setSelectedOption(selectedOption) {
        this.selectedOption = selectedOption;
        this.consumerOnChange?.(this.selectedOption?.value);
    }
}

eslint knows nothing about magic autobind for setSelectedOption and complains in every such method usage as event handlers.

image

We fixed this with proper use of arrow function instead of plain method

    setSelectedOption = (selectedOption) => {
        this.selectedOption = selectedOption;
        this.consumerOnChange?.(this.selectedOption?.value);
    }

As result we have clear readable validated code with correct binding without any autobind magic.
We clear tons such autobinds and makes our event handler as arrow functions.

Plese don't add autobind as default (@kubk @mweststrate).

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