search

mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.94k stars 640 forks source link

call lifecycle hooks after children are created #1952

Closed scytacki closed 1 year ago

scytacki commented 2 years ago

Previously, in some cases, the parent lifecycle hooks would be called before the children were created.

Fixes #1951 Maybe fixes #1680

Note: This will change the order of the hooks in some cases. You can see this change when references are accessed by looking at the changes in object.test.ts. The order should be changed in other cases too. I believe the order is still consistent with the intent. Ie afterCreate is always called before afterAttach for a node. And afterAttach should not be called until the parent is created. If you'd like I could add additional tests illustrating this order change.

jamonholmgren commented 1 year ago

@scytacki Thanks for the PR! I'm not as familiar with this part of the code base and will need to review it with the other core team members.

scytacki commented 1 year ago

@jamonholmgren have you had a chance to look at this more yet? We've been using it successfully in a large MST codebase without issue for about 3 months now.

jamonholmgren commented 1 year ago

@scytacki I appreciate you pushing this forward.

What is your response to this comment? https://github.com/mobxjs/mobx-state-tree/issues/1951#issuecomment-1284401374

I don't have a strong opinion on this, but I want to make sure whatever we do (especially if breaking changes are involved) is commonly acceptable to the community.

scytacki commented 1 year ago

@jamonholmgren thanks for pointing out that comment I missed it before. I've now responded.

I'm not sure what the project considers a breaking change. The behavior being changed (fixed) here is not something that was documented. So if the "contract" is to maintain what is documented, then this PR is not a breaking change. If the "contract" is to not change any behavior whether it is documented or not, then yes this is a breaking change.

The ways this changes the behavior are:

  1. The order of the hooks are called in a nested tree is no longer the same regardless of how a deep child node is accessed. In the tests of this PR it shows how the order changes if a reference to a deep child node is accessed directly instead of traversing the children from the root. The order after this PR is still consistent with the documentation though, and I'd argue it is also consistent with the reasonable expectation of a developer.
  2. afterCreate will always be able to access children. Perhaps a developer was using the fact that afterCreate previously would fail in some cases, and this developer was doing something special when it failed. However this failure was not documented, so maintaining this failure doesn't seem like something that needs to be done.

If you would rather maintain the current occasional failure of afterCreate and the order of the existing hooks, I could make a new PR that adds a new hook instead. I'm not sure what the name should be but perhaps something like afterChildrenAvailable. As a user of MST this wouldn't be my preference because it would be more complex to remember (and explain to others) about these 2 hooks.

coolsoftwaretyler commented 1 year ago

I think since we have another breaking change over in https://github.com/mobxjs/mobx-state-tree/pull/2039, we should give this one the same label and treat it as such.

I wish the exact order of operations had been documented, which would make the question clearer. But as it is, I know from experience that "minor" updates which change lifecycle updates in other packages have given me some nasty errors that were very hard to troubleshoot in production. I would prefer for MobX-State-Tree not to do that to our users, and if we have other breaking changes slated for release at some point, I think we should leverage that opportunity to bundle this change with it.

The added tests here are awesome, and will help answer future questions about this behavior and if/when we break again. Would also love to make sure we incorporate this into the updated documentation.

scytacki commented 1 year ago

We'd love to stop using our fork of MST and go back to the official version, so I might see if I can rework the PR to preserve the order of the hooks while still fixing the issue.

But even if I did that, would this still be considered a breaking a change because of my 2nd point above?

afterCreate will always be able to access children. Perhaps a developer was using the fact that afterCreate previously would fail in some cases, and this developer was doing something special when it failed.

coolsoftwaretyler commented 1 year ago

Hey @scytacki - @jamonholmgren and I are going to be meeting next week and I can ask him his opinion on the breakage. If you were to touch up this PR, I think we would end up in two possible states:

  1. We all agree that the change doesn't break (assuming it preserves the order of operations overall), but we may be ready to field some questions if people were handling the prior failure states in a special way. In this scenario, we'd be able to ship relatively quickly and get you off of your fork.
  2. We come to the conclusion that this modification is a breaking change, and should be handled as such. As I mentioned before, we have a handful of incoming breaking changes in open PRs, and I think we have some bigger ones planned for early 2024. We could still merge this change (or rebase) and y'all could point your dependencies at the specific GitHub commit for the interim, which would get you off the fork, and we'd be on a path towards v6, where you could get back in the main deployments from MST.

I think either scenario is reason enough to try and get this PR back in working order. If you're not interested in the longer term play here, I totally understand, and Jamon and I could try to source someone else to do the cleanup (or do it ourselves).

One way or another, I think this PR is important, and I'd like to end up merging it in. Would be stoked to have you continue helping out on it, but I am just happy you've been engaged as it is, and we can take on the responsibility of getting this thing closed out in the coming months (fingers crossed for sooner).

scytacki commented 1 year ago

Thanks @coolsoftwaretyler. I'll wait to see what you and Jamon decide. Preserving the older of hooks might add more code complexity that might not be worth it in the long run, but I'll take a closer look if you decide this would make the PR mergable and releasable this year.

As far as I know the PR is still in working order, so other than trying to preserve the order of the hooks I'm not sure what else I could help with. If there is something else it needs, let me know and I can try to find the time to address that.

coolsoftwaretyler commented 1 year ago

Hey @scytacki - Jamon and I spoke yesterday and feel good about merging this in and releasing with the next minor/patch (I think it'll technically be a minor release).

Since this'll be my first time running through the MST release process, I'm going to probably start it as an alpha candidate on the minor version. I plan to do that sometime this coming week.

For now, I'm going to merge this PR, and if you wanna keep an eye out for 5.2.0-alpha.1 on npm, that'll have this in there for ya, along with some other updates, fixes, etc.

I'll also be posting in discussions here about it to get feedback from folks. Hopefully with this PR and the other one you sent along, we can get y'all off your fork + improve MST reliability for all.

Really appreciate your time, and that you've stuck around for the long turnaround time on this.

scytacki commented 1 year ago

Yay! Thanks for looking at it and merging it in.

I forgot that we have one more addition to MST in our fork so we can't get off it yet, but hopefully we'll get a PR submitted for that soon.