chore(headless): dynamically load reducers #709
Conversation
samisayegh
requested review from
btaillon-coveo,
olamothe and
ThibodeauJF
as code owners
|
Pull Request Report PR Title ✅ Title follows the conventional commit spec. Bundle Size
|
| @@ -281,6 +291,11 @@ export class HeadlessEngine<Reducers extends ReducersMapObject> | |||
| } | |||
| } | |||
|
|
|||
| public addReducers(reducers: ReducersMapObject) { | |||
| this.reducerManager.add(reducers); | |||
| this.reduxStore.replaceReducer(this.reducerManager.reducer); | |||
There was a problem hiding this comment.
Small nitpick, but shouldn't the reducer manager be in charge of calling replaceReducer internally on the redux store, when add is executed ?
Seems natural to me that a manager would also manage that part.
There was a problem hiding this comment.
Hmm, I see ReducerManager's role as taking care of holding, adding and building the final reducer function.
I feel giving it the role of replacing the reducer on the store would be a tad too much. The reduxStore instance is part of this class. I could pass it into ReducerManager, but I feel it would be clearer to not do that,
| @@ -330,7 +338,7 @@ export function buildFacet( | |||
| const request = getRequest(); | |||
| const response = getResponse(); | |||
|
|
|||
| const isLoading = engine.state.search.isLoading; | |||
| const isLoading = getIsLoading(); | |||
There was a problem hiding this comment.
Is this change needed or just an improvement you saw as you were working on this ?
If it's needed, I'd like to understand why ;)
There was a problem hiding this comment.
Yep, I made this change to resolve a typescript error. Inside getters only, typescript doesn't correctly infer the state of the engine. It sees it as unknown.
Everywhere else in the controller, ts understands the correct type via the typeguard.
buildSomeController(
engine: Engine<unknown>
) {
// engine state is "unknown"
if (!loadReducers(engine)) {
throw Error
}
// if we are here, typescript understands that the engine state is that specified by the loadReducers type guard
return {
doSomeAction() {
// inferred engine state is still correct
},
get state() {
// here, typescript goes back to thinking the engine state is "unknown" for some reason.
}
}
}
There was a problem hiding this comment.
An alternative would be to do this, and update references to use resolvedEngine inside the controller:
export function buildFacet(engine: Engine<unknown>, props: FacetProps): Facet {
if (!loadFacetReducers(engine)) {
throw loadReducerError;
}
const resolvedEngine = engine;
Down-side is one would be able to use engine or resolvedEngine, except inside getters where only resolvedEngine would work.
There was a problem hiding this comment.
I created a report for the TS team: microsoft/TypeScript#43676
There was a problem hiding this comment.
This looks good but I'm still a bit confused at what this translates to on the ui library side. When instantiating the engine no reducers are required now I guess. I'm probably just not searching right but I don't see where they're loaded dynamically based on registered components.
| @@ -356,13 +371,17 @@ export class HeadlessEngine<Reducers extends ReducersMapObject> | |||
| }); | |||
| } | |||
|
|
|||
| private initReducerManger() { | |||
There was a problem hiding this comment.
| private initReducerManger() { | |
| private initReducerManager() { |
|
|
||
| constructor(private options: HeadlessOptions<Reducers>) { | ||
| this.initLogger(); | ||
| this.validateConfiguration(options); | ||
| this.initReducerManger(); |
There was a problem hiding this comment.
| this.initReducerManger(); | |
| this.initReducerManager(); |
| /** | ||
| * Adds the specified reducers to the store. | ||
| */ | ||
| addReducers(reducerMap: ReducersMapObject): void; |
There was a problem hiding this comment.
The argument is called reducerMap here and reducers on line 294. For consistency maybe they should be the same.
| const reducers = {...initialReducers}; | ||
|
|
||
| return { | ||
| get reducer() { |
There was a problem hiding this comment.
I would be more explicit to name this combinedReducer
Yea, once we convert all the controllers to the new approach, reducers would not need to be specified. The dynamic load happens at the start of the controller, where it adds the necessary reducers to the passed engine. |
|
Oh I see so this is added to the controllers. Sounds good 👍 thanks! |
The goal of this PR is to simplify Headless configuration. Rather than asking a developer to specify reducers, they would be added dynamically when instantiating a controller. Some benefits of the approach are:
reducersoption when instantiating an engine would eventually become optional.searchAppReducersare included, whether or not the feature set is being used.Inspiration: https://redux.js.org/recipes/code-splitting#using-a-reducer-manager