-
Notifications
You must be signed in to change notification settings - Fork 34
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
refactor(atomic): split the atomic store into composable parts #4806
base: master
Are you sure you want to change the base?
Conversation
Pull Request ReportPR Title✅ Title follows the conventional commit spec. Live demo linksBundle Size
SSR Progress
Detailed logssearch : buildInteractiveResultsearch : buildInteractiveInstantResult search : buildInteractiveRecentResult search : buildInteractiveCitation search : buildGeneratedAnswer recommendation : missing SSR support case-assist : missing SSR support insight : missing SSR support commerce : missing SSR support |
e350c15
to
8e4b709
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a breaking change? Custom components can rely on store functions / attributes.
Maybe it is but this could be considered as a bug. For example, all of those should not be available to use in the
|
export interface CommonStore<StoreData> { | ||
state: StoreData; | ||
onChange: <PropName extends keyof StoreData>( | ||
propName: PropName, | ||
cb: (newValue: StoreData[PropName]) => void | ||
) => () => void; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only real breaking change imo would be that I removed the get
and set
options. As written in the stencil-store documentation. These options are only there to support IE11 since it did not support Proxy...
I think I should re add those in case someone used .get() and .set() for valid store keys in custom components. And remove them in v4.
old interface
export interface CommonStencilStore<StoreData extends AtomicCommonStoreData> {
state: StoreData;
get: <PropName extends keyof StoreData>(
propName: PropName
) => StoreData[PropName];
set: <PropName extends keyof StoreData>(
propName: PropName,
value: StoreData[PropName]
) => void;
onChange: <PropName extends keyof StoreData>(
propName: PropName,
cb: (newValue: StoreData[PropName]) => void
) => () => void;
}
only real breaking change would be this imo https://github.com/coveo/ui-kit/pull/4806/files#r1904333364 |
@fbeaudoincoveo I personally feel comfortable making all the possible breaking changes in this PR. Even if a custom component was using the store, They should have never used those I removed in this PR. Someone using AtomicRecs should have never used I will re add the |
36ab130
to
4d417a6
Compare
https://coveord.atlassian.net/browse/KIT-3814
The atomic store was messy. The common store was way too big. There was no need for all that stuff in every store.