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

fix #3747 #3748

Merged
merged 4 commits into from
Aug 31, 2023
Merged

fix #3747 #3748

merged 4 commits into from
Aug 31, 2023

Conversation

mweststrate
Copy link
Member

Code change checklist

  • Added/updated unit tests
  • Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (yarn mobx test:performance)

@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2023

🦋 Changeset detected

Latest commit: 60d4072

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mobx Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mweststrate
Copy link
Member Author

Fails autoruns rescheduling:

Screenshot 2023-08-31 at 10 04 20

if (globalState.inBatch && this.batchId_ === globalState.batchId) {
// Called from the same batch this atom was created in.
return
if (globalState.inBatch && this.batchId_ !== globalState.batchId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!globalState.inBatch || this.batchId_ !== globalState.batchId
If not in batch mutation always updates version immediately.
If in batch only if it's different.

@urugator
Copy link
Collaborator

urugator commented Aug 31, 2023

Btw the state version is currently relevant only for observer, so with the proposed fix, it should not interfere with inner workings of mobx like autoruns scheduling etc in any way.
However, no matter the implementation, we still assume the following:
If you create and mutate an observable in the same batch, then there should be no need to notify any derivations except the computeds created inside that same batch. Because there should be no way to somehow leak that observable to some different derivation (except that computeds) that isn't currently running. Eg. even if you consider nesting derivations like:

autorun(() => {
  // batch id 1
  const o = observable.box(0);
  autorun(() => {
    // This won't run until the outer `autorun` finishes
   // batch id 2
    o.get();
  });
  o.set(1); // no point in reporting change to nested autorun, it's not subscribed to `o` yet.
})

If there is a case, that I am not aware of, where that isn't true, then there is a problem.

@mweststrate
Copy link
Member Author

Heh, the fix ends in state o.x === 3 instead of o.x === 2, whereas 5 is (correctly imho) expected so still doesn't seem to fully reschedule itself correctly?

@@ -68,7 +68,7 @@ export class Atom implements IAtom {
* Invoke this method _after_ this method has changed to signal mobx that all its observers should invalidate.
*/
public reportChanged() {
if (globalState.inBatch && this.batchId_ !== globalState.batchId) {
if (!globalState.inBatch && this.batchId_ !== globalState.batchId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& => ||

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh lol I should try to read before copy pasting next time. That fixes the test. However, the autorun doesn't initially reschedule itself (see the comment in the test), I think this used to be the case in the past, or am I mistaken here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this used to be the case in the past, or am I mistaken here?

I don't know if this was the case in the past, but it wasn't the case when I wrote this test, otherwise there wouldn't be that extra o.x++ outside the autorun to nudge it and it would fail, because o.x would end up being 6. I think I was probably a bit surprised as well when writing it. However, if there is some problem here, it was there before introducing this state version business.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check, I'll land this, as it sounds that might be a separate issue, if it is one.

@mweststrate mweststrate merged commit c8d9374 into main Aug 31, 2023
1 of 2 checks passed
@github-actions github-actions bot mentioned this pull request Aug 31, 2023
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.

2 participants