Skip to content

create optimistic proxy #423

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

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
@@ -134,8 +137,11 @@ export function createIsographEnvironment(
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