[BB pr#39] KIT-40 [Demo/POC] How to extend the Engine

[coveobot]

Pull request 🔀 created by @ThibodeauJF on 2020-06-12 12:23
Last updated on 2020-06-17 21:30
Original Bitbucket pull request id: 39

Participants:

• bitbucket user coveo-anti-throttling_02
• @btaillon (reviewer)
• @olamothe (reviewer) ✔️
• @samisayegh (reviewer) ✔️
• @ThibodeauJF

Source: Commit 862d557e9926 on branch KIT-40> Destination: https://bitbucket.org/coveord/ui-kit/commits/0279f447333a on branch master
Merge commit: https://github.com/None/commit/118349977e43

State: MERGED

Everything under the /addon-example folder is not intended to be merged (or maybe it should be? as a code example, with more documentation).

I modified a few thing about the Engine’s type to make it possible to extend it without too much hassle. Basically, when you pass some custom made reducers to the addons option, it will “magically” add a piece of state!

I made it possible to extend the Component type too with an extended Engine.

This example would work well for customers and other teams at the company. A question right now is where we would put the code? I see 2 ways:

1. Keep it all inside the headless package and let other teams create and export custom Engines (e.g., SalesforceEngine, CommerceEngine). The codes-splitting should take care of only importing the add-on features if made explicit.
2. Make another package in the ui-kit repo that would import headless and extent it. That would be pretty useful if a team wants control over the release process.

FEEDBACK!! 🙏 🙏 🙏

https://coveord.atlassian.net/browse/KIT-40 & https://coveord.atlassian.net/browse/KIT-28

‌

[coveobot]

@ThibodeauJF commented on 2020-06-12 12:30

Outdated location: line 25 of packages/headless/src/addon-example/pets-engine.ts

Typescript autocomplete catFacts correctly here. We don’t need to specify a “state type” anywhere.

[coveobot]

@ThibodeauJF commented on 2020-06-12 12:31

Outdated location: line 31 of packages/headless/src/app/headless-engine.ts

This piece of code ReducersMapObject<AdditionalState> is how Typescript infers the AdditionalState's type from the passed reducers correctly without additional work.

[coveobot]

@ThibodeauJF commented on 2020-06-12 12:33

Outdated location: line 2 of packages/headless/src/index.ts

Maybe we shouldn’t export the Engine type but only the ReduxEngine? (here renamed to HeadlessEngine and exported Engine for simplicity)

[coveobot]

@ThibodeauJF commented on 2020-06-12 12:34

Outdated location: line 132 of packages/headless/src/app/headless-engine.ts

This is how we get the complete extended state. By adding the 2 types.

[coveobot]

@olamothe commented on 2020-06-12 20:11

I think #2 would be the correct way to go about it, otherwise the release process might get “crowded”, it will be harder to release some integration individually.

Now, if we take what you have here a step even further:

engine could be a separate package. Just something that handle a configuration to an organization (token, orgID), a redux store and a way to subscribe to the store. Super simple and now much happening in there. We try to design something that could be used by ALL api: token renewal ? Client side throttling ? A generic httpClient class ? That exposes how we end up doing http request/possibly expose plumbing to tap into the “executing an http request pipeline” and allow parameters modification ?

Then we’d have a search-actions package, that would export/contain everything we currently have under src/headless/api and src/headless/features. AKA: All the “redux actions” we create and need to power search pages. This is where we would export the shape of the store that we need to power a search page.

Then we’d have a search-engine package. It would import from engine, configure the things needed to call “search API” (token/orgID/what’s the endpoint etc.) It would also import (and re-export) from search-actions , and would contain all the headless-components as they exist right now. So, in atomic, we’d actually only import directly from search-engine.

‌

Now, let’s say salesforce/service now team need to create something for case deflection, which would hypothetically need to call the “customer service API” to power the main feature of the package, and also the search-api because they need a query suggestions feature that is not exposed through the customer service API .

They would first create a customer-service-actions package. It would export/contain all their own features / redux-actions and the shape of their store. They could import actions from search-actions package if they need to. I think this is where this could be pretty powerful and flexible: you would be able to import/mix and match other redux actions from other packages that you need to create your own.

Then they would create a customer-service-engine package. It would be similar to our search-engine package: They would import from customer-service-actions, they would create their headless-components. And their visual library would then import directly from customer-service-engine.

Repeat for commerce,

We could even try to think how this could be used to create a customer-service-and-also-commerce-engine kind of meta package ? Since it could import/mash together both service underneath in some hybrid scenario ?

‌

Now would that architecture makes sense, or would it be complicating things “needlessly” ?

Just typing it out made me doubt that this might be over engineered. Would we be jumping all across the places in different packages to be able to do our day to day job ? Is that inventing a problem that does not even exist yet ?

What you have right now also seems pretty powerful. And I’m going to approve for this reason. But something to discuss…

[coveobot]

@olamothe approved ✔️ the pull request on 2020-06-12 20:11

[coveobot]

@ThibodeauJF commented on 2020-06-12 21:35

Just typing it out made me doubt that this might be over engineered. Would we be jumping all across the places in different packages to be able to do our day to day job ? Is that inventing a problem that does not even exist yet ?

Reading it I feel this way too, maybe it’s a bit too much over-engineering for now.

We only have to define what is “Core”, meaning what features (slices) and what component would all engines in all Coveo teams need. That core would stay in the current headless package. Let’s call it headless-core

Then we could have an Engine with all the stuff for a search page (what headless is now currently). Let’s call it headless-search

Then for all product types (commerce, salesforce, service, etc.) they could have their own package too, they could import what they need from headless-core and maybe headless-search and manage their own release/versioning.

I think we should keep it 1 package = 1 product or tool (like bueno). That would prevent jumping around for most of the time.

I think that this could all be possible with what we have in this PR (using addons) we would have to set up a test-package to have a better idea.

‌

[coveobot]

@samisayegh commented on 2020-06-15 18:38

Outdated location: line 2 of packages/headless/src/index.ts

I think it’s nice for developers to have access to the type if they want to create their own engine implementations. I also think HeadlessEngine is good name for the default/native engine, especially if we one day export other kinds of engines from this package or others.

[coveobot]

@samisayegh commented on 2020-06-15 18:39

Outdated location: line 4 of packages/headless/src/components/component/headless-component.ts

Why does the engine need to be publicly exposed?

[coveobot]

@samisayegh commented on 2020-06-15 18:40

Outdated location: line 13 of packages/headless/src/app/root-reducer.ts

Seems to me that this function should accept a generic type, since it is no longer returning a HeadlessState when the additionalReducers argument is passed.

‌

[coveobot]

@samisayegh commented on 2020-06-15 18:42

Outdated location: line 132 of packages/headless/src/app/headless-engine.ts

Could the cast be removed if we allowed passing a generic to the configureStore function instead?

[coveobot]

@samisayegh commented on 2020-06-15 19:32

Hmm, personally I have the opposite view. I don’t like the idea of developers/users needing to install multiple packages to use different apis.

What features belong together is blurry and I don’t see why we should decide this for our users.

Then there is the aspect of wiring packages. Not so difficult for one type as shown in this PR, but once you have 3 or 4 different Engine state types, along with the required reducers (not necessarily easy to figure out), then I can see it becoming challenge. I think Olivier your comment here

create a customer-service-and-also-commerce-engine kind of meta package

comes from good intentions, trying to hide the wiring. However, it does not scale, as the permutations will grow very quickly.

‌

I think all functionality that we code should exist in the headless package. Using a plugin model one could pick-and-choose when configuring the reducer. As a helper, we can expose a function that returns all reducers.

// contains everything.
const defaultReducers = {
    a: aReducer,
    b: bReducer
}

new HeadlessEngine({ addons: defaultReducers() }) // I think 'addons' should be called 'reducers'

If someone wants to optimize for bundle size, they can build their own reducers object. We can also expose other helpers that return combinations of reducers based on the apis “commerceReducers”, “caseDeflectionReducers“ etc. that can be composed as desired.

import { commerceReducers, caseDeflectionReducers} from '@coveo/headless'
const myReducers = {
    ...commerceReducers(),
    ...caseDeflectionReducers(),
}

‌

For the typescript, it would be cool if the developer did not have to pass anything in and the engine.state has the correct type inferred from what reducers they pass in. Not sure if there is a limitation by typescript here though,

[coveobot]

@olamothe commented on 2020-06-15 19:38

I think all functionality that we code should exist in the headless package. Using a plugin model one could pick-and-choose when configuring the reducer

My main concern with this is the release process might become pretty complicated, and a lot of “hey guys when is the release for headless, I have this super important client/feature that is waiting on it”.

But heh, if it comes to that then I guess we have a “nice problem on our hand” :slight_smile: Because it means many teams are using the project 😉

And this can be mitigated by having proper validation tool/automated test, making the release process very quick.

So my proposition was in the hope of trying to scale the humans, not the code 😃 Which I think is just as hard, if not more…

[coveobot]

@samisayegh commented on 2020-06-15 19:45

Ok, I see. We could mitigate such risks in different ways. Automated tests would be great. If unavailable, we can also publish an alpha package on every merge commit, or every night, for people who need changes in a short window.

[coveobot]

@samisayegh commented on 2020-06-15 19:53

Outdated location: line 66 of packages/headless/src/app/headless-engine.ts

Following on the above comment, I think it’s better if the Engine interface accepts the “full state” as a generic, rather than the additional state. This does not force a developer to use the full HeadlessState if they do not want to. It also simplifies repeating the AdditionalState & HeadlessState type.

‌

[coveobot]

@ThibodeauJF commented on 2020-06-16 13:20

Okay, so code every features & component inside headless, expose everything, even the reducers?

Then the issue comes to: the customer has to know what Component deals with what reducers, actions etc. I can see complexity here….

What we could do is have one BIG core package that has everything & expose everything. But either have wrapper packages that make it more elegant or find another clever way to organize export.

The hardest thing is to get the typescript definitions to follow.

‌

[coveobot]

@ThibodeauJF commented on 2020-06-16 21:24

Thanks for the feedback guys!

So now, following this, I infer the state from the reducers that are passed to the engine. I export the current reducers as “core reducers” and use those as default. So a user is now able to pick & chose the reducers he requires.

One limitation I see is that we use the HeadlessState interface throughout the code and export it. This causes the following issues:

1. For components, since they have an engine as parameter with the HeadlessState, it will require all the coreReducers for it to work (Typescript-wise).
2. We export a documented HeadlessState, maybe we should not and only infer the state from the reducer. I’m not sure we can keep the docs on them…

Also, of course creating an engine with no “core-reducers” will break the API for example.

Maybe we should have both “core-reducers”, that are obligatory, and “search-reducers” that are for the search. Components should also specify what reducers are required to be included in the engine.

Anyways, still lots of questions 🤔 🤔 🤔

[coveobot]

@samisayegh commented on 2020-06-16 22:18

Location: line 140 of packages/headless/src/app/headless-engine.ts

Is this cast still needed? If so, is there something we could do with configureStore to get the proper type?

[coveobot]

@samisayegh commented on 2020-06-16 22:20

Location: line 7 of packages/headless/src/app/headless-engine.ts

Nice, did not know that RTK exported these types.

[coveobot]

@samisayegh commented on 2020-06-16 23:01

On the question of core vs search reducers, I think these helpers would be useful to expose one day, but I do not think it’s important at this stage. I think what is currently called coreReducers should instead be allReducers. As we build out headless, we’ll add everything in there. When we have better information of what the new apis will require, we will be able to separate core, search, commerce etc. subsets.

For the default value of the reducers option, I don’t think we should have one. Jf we do, the bundlers will not be able to tree-shake reducers referenced in the default value. If the default is allReducers, then we cannot tree-shake anything. If the default is coreReducers, well we do not know what to consider core just yet. Also, removing something from core at a later point would be a breaking change.

For #2 on the reference documentation, I believe we can still get jsDoc if one adds it above the property in the object. Would something like this work? e.g.

const coreReducers = {
    /** documentation for query reducer */
    query: queryReducer
}

type coreReducersType = typeof coreReducers;

‌

For #1, I see … interesting problem. It makes sense too in that a class usually only needs a slice of state, not the whole thing. Stricter typing would also guide a dev as to what reducers they need to configure for the headless class to work correctly. If we had types for each of the reducers, I think we could solve this problem. Could something along these lines work?

class Pager extends Component<Engine<PaginationReducer>> {}

class SearchBox extends Component<Engine<QueryReducer & QuerySuggestReducer>> {}

‌

Overall, this PR is looking good. Nice investigations 💯

[coveobot]

@samisayegh approved ✔️ the pull request on 2020-06-16 23:02

[coveobot]

@ThibodeauJF commented on 2020-06-17 12:21

Location: line 140 of packages/headless/src/app/headless-engine.ts

Yes, it returns an ugly CombinedState

[coveobot]

@ThibodeauJF commented on 2020-06-17 12:25

Thanks!

For #1, yes we’ll have to divide the reducer maps and specify them in each components, not necessary for now, but once we extract the core yes.

For #2 I found the way to do it, it won’t be an issue for now (assigning ReducersMapObject<HeadlessState> to the reducers)

[coveobot]

Bitbucket user coveo-anti-throttling_02 commented on 2020-06-17 12:26

Bundle Size Report

File	Compression	Old (kb)	New (kb)	Change (%)
dist/browser/headless.js	bundled	115	114.4	-0.5
	minified	57.7	57.8	0.2
	gzipped	18.5	18.5	0.1
dist/browser/headless.esm.js	bundled	111.6	111	-0.6
	minified	63.7	63.7	0.1
	gzipped	19.2	19.3	0.2

[coveobot]

@samisayegh commented on 2020-06-17 12:59

I was thinking some more end of day yesterday …

“searchReducers” is probably better than “allReducers” for now. In the longer term, this will avoid bloating early headless users, while keeping the naming representative.

[coveobot]

@ThibodeauJF commented on 2020-06-17 13:40

I’d want to rename that search reducer/slice then 😉