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

Selfless model #2145

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Selfless model #2145

wants to merge 5 commits into from

Conversation

BrianHung
Copy link
Contributor

What does this PR do and why?

This is a draft PR that attempts to implement the selfless model described in #1683. It is incomplete and would love if someone took this over! :)

@coolsoftwaretyler
Copy link
Collaborator

Hey @BrianHung. Looks like you've been busy, haha.

This is very intriguing. I thought at first this might be like, a new model that was selfless (option 3a from @k-g-a), but is my initial reading correct to say this is a fully breaking change, and no model in MST would use self anymore?

Two more questions:

  1. Have you tested this code to see if the performance improvements are real
  2. How dedicated are you to this project? If we move forward with this implementation, it's going to require a lot of documentation updates and much more. I'm up for it. This is a great way to achieve better performance and improve docs at the same time, I'm just curious about your availability to help in that process.

@BrianHung
Copy link
Contributor Author

BrianHung commented Feb 18, 2024

a new model that was selfless

oh in hindsight I definitely should’ve just created a separate model 😅

my initial reading correct to say this is a fully breaking change, and no model in MST would use self anymore

yep that’s correct

Have you tested this code to see if the performance improvements are real

Not yet; have just been re-using the old tests for the regular model for correctness.

How dedicated are you to this project?

I can’t guarantee any time. I drafted this PR in hopes someone else can look and use it as a foundation for a merge worthy PR. ☺️

When I have free time (maybe in a couple months when I’m not as backlogged), I will probably submit a different PR with selfless being its own model as mentioned earlier. That would also let us test performance differences more easily and help migrate people in the future.

But in the meantime, yeah selfless model seems critical closing the MST-MobX performance gap and would love if someone were to get to it faster than I can right now. 💖

@coolsoftwaretyler
Copy link
Collaborator

Makes sense! I think this PR has had the intended effect. I've read the referenced thread a few times but never knew where to start. This work will jumpstart that.

I can champion this and find other folks to work on it, or pick it up myself.

I think we'll want to ship this as a new type of model. That way we can avoid a breaking change, at least while we experiment with it.

Another heads up: the master branch has diverged a lot from where this PR was. We moved away from the monorepo.

This work is easy enough to copy over to the new directory structure, but if you've been working off a fork, you might want to pull down our changes. Lots of directory path changes, and I think we're gonna do a big TypeScript update shortly.

Thanks again for this work. I think it's going to help move the needle.

@techknowfile
Copy link

Hey! I was the original reporter of #1683. I have since moved on to another job that is entirely backend, but I still think of MobX often. I wonder if I might be able to find some extra time to get up to speed with the fundamentals of mobx and mst so that I could help contribute to making this work.

@coolsoftwaretyler
Copy link
Collaborator

Hey @techknowfile - you'd be a welcome help!

If you decide to pitch in and want some hands on help getting up to speed on the codebase, feel free to email me. My email is either in my GitHub profile or it's easy to drive/find based on my personal website.

Would love to facilitate your efforts if that's helpful to you.

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.

3 participants