[BB pr#6] KIT-4 Add "test" script in build process

[coveobot]

Pull request 🔀 created by @ThibodeauJF on 2020-04-28 11:54
Last updated on 2020-04-28 20:26
Original Bitbucket pull request id: 6

Participants:

• @btaillon (reviewer)
• @olamothe (reviewer)
• @samisayegh (reviewer) ✔️
• @ThibodeauJF

Source: Commit 26504a5ee2a3 on branch KIT-4> Destination: https://bitbucket.org/coveord/ui-kit/commits/ae2b02a48e6f on branch master
Merge commit: https://github.com/None/commit/0c40bc1d43c1

State: MERGED

Added some UTs too to give an example.

https://coveord.atlassian.net/browse/KIT-4

‌

[coveobot]

@samisayegh commented on 2020-04-28 18:04

Outdated location: line 6 of packages/headless/src/features/query/querySlice.test.ts

As we add new properties to the default state object, would we need to update this object to keep the test on line 11 passing?

If so, should the initial/default state be a function in a source file, imported and called here?

[coveobot]

@samisayegh commented on 2020-04-28 18:21

Outdated location: line 7 of packages/headless/src/features/query/querySlice.test.ts

We did not have time to discuss alternative points of view after the presentation but … maybe we can do that here. Summoning @{557058:6fb10fac-dc5b-4d35-b648-4b2d42952452} .

I believe the language of the store should match (or resemble when not possible) the language of the search-api. I think this would reduce the terminology someone new to Coveo, or even just new to Headless, would need to know and remember. For example:

1. Someone inspecting the network tab could easily map the response to our store (and vice-versa). If someone wants to set the q in outbound requests, they do not need the extra step of learning that to do that in headless, they need to set the expression.
2. Someone trying to extend Headless can reference the search-api documentation directly. If someone wants to see the possible values of sortCriteria, it’s helpful if we use the same name in our Store and Actions, rather than a new name like “order”

‌

What do you think?

[coveobot]

@olamothe commented on 2020-04-28 18:30

Outdated location: line 7 of packages/headless/src/features/query/querySlice.test.ts

• Yes, we should try to stick closely to the API names/terminology, it helps comprehension a lot/reduce overall confusion, exactly like you described @{557058:ef2198d5-03b4-4d5f-86e7-0a23e51f4f14}
• However, in some case, the API name are arguably poor. q is a good example I think, as well as aq, cq, dq, lq etc. It’s entirely possible that if we were to start new API design from scratch, those could be renamed. That’s the type of thing we could discuss with Search API team/Martin for the API redesign, to help them identify stuff we feel is badly named, see if they agree or not.
• And so, any API changes will impact the shape of the state object we expose. Because, if we decide to name it q and in 5 months, in the new API call it’s now called expression, we’d be in contradiction with the first point.
• So my suggestion is, for now, to stick very closely to the Search API terminology that already exists (so, we would use q ). And if/when the API is redesigned and the new name does not match the shape of the store, we would do a major version breaking change in our library (which is arguably annoying for consumer of the library, but… 🤷‍♂️ ).

‌

[coveobot]

@samisayegh commented on 2020-04-28 18:32

Outdated location: line 20 of packages/headless/src/features/query/querySlice.test.ts

Related to the above comment, I think the action could be shortened to updateQuery and still be clear to a new user.

[coveobot]

@samisayegh commented on 2020-04-28 18:39

Outdated location: line 12 of packages/headless/src/features/results/resultsSlice.test.ts

Similar to the above comment, if list holds the results, I think it should be named results to match the search api response.

The other properties are the same which is great.

[coveobot]

@samisayegh commented on 2020-04-28 18:51

Outdated location: line 3 of packages/headless/src/utils/fakeResults.ts

A pattern that I find really flexible is setting the argument to a Partial object of the full return type. This allows a consumer of the function to set any combination of values upon creation. For the name, I think get is good, but I find words like build or create better capture what is happening. e.g.

export function buildFakeResult(config: Partial<SearchResult> = {}): SearchResult {
    return {
        default keys and values,
        ...config
    }
}

‌

[coveobot]

@samisayegh approved ✔️ the pull request on 2020-04-28 18:52

[coveobot]

@ThibodeauJF commented on 2020-04-28 19:07

Outdated location: line 7 of packages/headless/src/features/query/querySlice.test.ts

@{557058:6fb10fac-dc5b-4d35-b648-4b2d42952452} @{557058:ef2198d5-03b4-4d5f-86e7-0a23e51f4f14} I agree with mostly all this. I think we should aim to stick to the API when we are “OK” with the naming, and for the stuff like q I think we should go with something clear and try to influence the API to change those poorly named params in their newer calls.

Also what is a query? Is it everything containing the q, aq, lq, facets, etc. Or is it just the basic expression, it can be highly confusing and we should not bring that debt into headless.

Looking at the q Swagger, it says: “The basic query expression”. I’ll switch it to q for now but when we get to implementing those search calls in the future we might want to discuss it with API & doc first.

[coveobot]

@ThibodeauJF commented on 2020-04-28 19:20

Outdated location: line 3 of packages/headless/src/utils/fakeResults.ts

Good idea!

[coveobot]

@ThibodeauJF commented on 2020-04-28 19:25

Location: line 1 of packages/headless/typings/coveo-headless-engine.d.ts

A bit irrelevant now until we develop “real” public API

[coveobot]

@samisayegh commented on 2020-04-28 19:30

Outdated location: line 7 of packages/headless/src/features/query/querySlice.test.ts

Yes, since we will likely be the first users of the new api, we should be in on the discussions and Google Doc design documents when the planning starts, to bring a different perspective on what is exposed, and how those exposed things are named.