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(entity-store): split into asset and entry map to prevent id collision #406

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

chrishelgert
Copy link
Contributor

@chrishelgert chrishelgert commented Nov 7, 2023

Entry and Asset can have the same ID, therefore we should split up the entities into two maps (one for asset, one for entry)

@chrishelgert chrishelgert requested a review from a team November 16, 2023 09:30
@chrishelgert chrishelgert marked this pull request as ready for review November 16, 2023 09:30
const store = new EntityStore({ entities: [patchedAsset, patchedEntry], locale });

const [resolvedAsset, resolvedEntry] = await Promise.all([
store.fetchAsset('1'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens with?

public get entities() {
  return [...this.entryMap.values(), ...this.assetMap.values()];
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

() is it possible to have duplicates? EntityStore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes there it is possible to have duplicate sys.id, but you also have the sys.type to differentiate between them, so that should be fine :)

@chrishelgert chrishelgert merged commit 8689a1e into main Nov 17, 2023
3 checks passed
@chrishelgert chrishelgert deleted the fix/entity-store branch November 17, 2023 14:07
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