Skip to content

Conversation

@paula-stacho
Copy link
Collaborator

@paula-stacho paula-stacho commented Sep 30, 2024

Description

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 force-pushed the sharded-state branch 2 times, most recently from cebd5db to 9c255d6 Compare October 1, 2024 11:26
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Oct 7, 2024
@paula-stacho paula-stacho marked this pull request as ready for review October 7, 2024 09:33
@paula-stacho paula-stacho removed the wip label Oct 7, 2024
Comment on lines +90 to +94
const store = setupStore(options, services, connectionInfo);
return renderWithActiveConnection(
<Provider store={store}>{component}</Provider>,
connectionInfo
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FYI, we specifically added these helpers so you don't need to manually wire providers and run activate methods

@gribnoysup
Copy link
Collaborator

GitHub collapsed the reducer file and I only looked at the changes there today while revieweing again, sorry for not including this in my initial review

},
});
await store.dispatch(fetchClusterShardingData());
// expect(store.getState().status).to.equal('SHARD_KEY_CORRECT'); // no idea why this does not work
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the state not matching or the check? What state do you see here? (we probably want to either remove this comment altogether or resolve, a bit weird to just leave this comment 🙂)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was seeing NOT_READY. Putting the check behind waitFor helped, but I'm not sure why it was needed 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it's because this fetch action is not the one that actually will update the state, and the one that will is not awaited inside this one:

image

Not sure if it's this intentional or not

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah right! I was looking at this code and read it wrong 🤦 outside of tests it currently doesn't make difference if we wait for this one, since the fetchClusterShardingData is not awaited.

@paula-stacho paula-stacho merged commit 58fb9f5 into main Oct 9, 2024
@paula-stacho paula-stacho deleted the sharded-state branch October 9, 2024 12:07
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.

4 participants