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

feat(compass-global-writes): invalid and mismatching shard key state COMPASS-8278 #6371

Merged
merged 7 commits into from
Oct 17, 2024

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Oct 16, 2024

Description

Adding two new screens, handling the invalid and mismatching shard key states. This one was pretty straight-forward. Content taken from current data-explorer, updated to Compass/LG components.

Screenshot 2024-10-16 at 16 36 44 Screenshot 2024-10-16 at 16 35 21

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@paula-stacho paula-stacho changed the title COMPASS-8278 feat(compass-global-writes): invalid and mismatching shard key state COMPASS-8278 Oct 16, 2024
@github-actions github-actions bot added the feat label Oct 16, 2024
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Oct 16, 2024
@paula-stacho paula-stacho marked this pull request as ready for review October 16, 2024 16:30
Comment on lines 62 to 64
if (!shardKey) {
throw new Error('Shard key not found in ShardKeyMismatch');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super splitting hairs here, caught my eye and I started thinking whether or not it would be better to throw in render or in connect function for this: this code absolutely makes sense, at the same time it's a property of the store that this state might be undefined and depends on the state status, as we are not actually passing the status to the component (totally makes sense!), maybe the status check and a throw should be kept to the connect function, then we can make this property required on the component? Idk, just really some thoughts about minute details of how we should be doing mapping of state in similar cases, what do you think? 🙂

Copy link
Contributor Author

@paula-stacho paula-stacho Oct 17, 2024

Choose a reason for hiding this comment

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

That makes sense actually. Even though theoretically in this particular setup the error could be due to wrong routing in the index component, it makes sense that the component would just expect matching & valid state. It is the connector's responsibility to map the state, so it should be the one to notice that the state is wrong. Thanks for pointing it out!

@paula-stacho paula-stacho merged commit 0094783 into main Oct 17, 2024
28 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8278 branch October 17, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants