Skip to content

Conversation

PatrykWalach
Copy link
Member

No description provided.

return storeRecord;
}

optimisticStoreRecord = target[p] ??= {};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a part that's bugging me, if we have a record in original store we create a new record in optimisticLayer just in case we need to write to it.

test optimisticProxy
refactor test
refactor proxy 

All the proxies work in the same way:
if value present in `A`
return value
otherwise return value from `B`
fix logic error
remove unused import
logFunction?.({
kind: 'EnvironmentCreated',
});
let optimisticLayer: OptimisticLayer = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rust brain has gotten to you
Also this should be caught by eslint?? (Also below)

}
}

resetOptimisticLayer(environment);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this just be environment.optimisticLayer = {} or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it shouldn't because then I have to create new proxy, but maybe it should because otherwise the old proxies point to the records that's been deleted, thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

startUpdate run inside startUpdate would surface this issue.

startUpdate((data) => {
  startUpdate(()=>{}) // this is merging old changes and assigning new object to `optimisticLayer` 

  data // this still points to the old layer 
})

@rbalicki2
Copy link
Collaborator

Okay, I thought about this more deeply. Take a look at #427

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