[BB pr#67] KIT-64 Create result template atomic component

[coveobot]

Pull request :twisted_rightwards_arrows: created by @ThibodeauJF on 2020-07-08 19:54 Last updated on 2020-07-20 11:29 Original Bitbucket pull request id: 67

Participants:

• @acote-coveo (reviewer)
• @ThibodeauJF
• @btaillon (reviewer)
• bitbucket user coveo-anti-throttling_02
• bitbucket user Kyu Shim (reviewer)
• @olamothe (reviewer) :heavy_check_mark:
• @samisayegh (reviewer)

Source: Commit ad8fcb04379f on branch KIT-64> Destination: https://bitbucket.org/coveord/ui-kit/commits/af6822c6396a on branch master Merge commit: https://github.com/None/commit/0becde3c9d03

State: MERGED

Validation of the props could still be improved, but since those props are a bit more complex, it might take a bit of work inside Bueno.

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

‌

[coveobot]

@olamothe commented on 2020-07-08 20:19

Outdated location: line 34 of packages/headless/src/features/result-templates/result-templates-manager.ts

This could be annoying in a way:

Say I am a user of atomic, I code my template and I “assume that I’m going to be receiving a result with raw.foo = ‘bar’” , because I know what my index contains.

And so I code my condition by assuming that raw.foo will always be defined/not null etc.

It’s debatable wether it’s a good idea that I do that and that I should code more defensively, but… anyway.

Because of this check here, I would have to always add defensive code in my template to make sure I’m not being called by a “mockResults”, which does not contains the property.

‌

Which could be annoying and cause some “wtf?” moment for clients.

‌

[coveobot]

@olamothe commented on 2020-07-08 20:24

Outdated location: line 33 of packages/atomic/src/index.html

Maybe we could eventually offer some kind of global function in atomic to perform the job of correctly selecting the correct atomic-result-template component, and passing in conditions ?

For now, this seems okay though. Not “super elegant/clean”, but okay, I guess.

[coveobot]

@ThibodeauJF commented on 2020-07-08 20:30

Outdated location: line 34 of packages/headless/src/features/result-templates/result-templates-manager.ts

Yes I get it, I was on the fence for the mockResult stuff, I can remove that check and just verify if it’s a function, should be sufficient for most cases.

[coveobot]

@ThibodeauJF commented on 2020-07-08 20:35

Outdated location: line 33 of packages/atomic/src/index.html

Yes, but I’d be curious to see how that would work with Stencil. That’s the way to pass “complex” props to a custom element. https://stenciljs.com/docs/javascript

I did not put an example for fieldsMustMatch, but they could be made simpler and easier to pass as a single string if needed e.g.: fields-must-match=”author=Author1,Author2”

[coveobot]

@olamothe approved :heavy_check_mark: the pull request on 2020-07-08 20:41

[coveobot]

Bitbucket user coveo-anti-throttling_02 commented on 2020-07-08 21:10

Bundle Size Report

File	Compression	Old (kb)	New (kb)	Change (%)
dist/browser/headless.js	bundled	188	188.6	0.3
minified	91.5	91.7	0.3
gzipped	27.3	27.4	0.2
dist/browser/headless.esm.js	bundled	182.5	183.1	0.3
minified	104.1	104.4	0.3
gzipped	28.8	28.9	0.3

[coveobot]

@samisayegh commented on 2020-07-08 21:48

Location: line 70 of packages/atomic/src/components/atomic-result-list/atomic-result-list.tsx

Why do we need to set the id to result.uniqueId?

Also, since we are injecting the string as innerHTML, we need to protect against XSS attacks. Maybe Mustache has something to handle this?

[coveobot]

@samisayegh commented on 2020-07-08 21:49

Outdated location: line 17 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

We could initialize priority to a default value here (e.g. 0).

[coveobot]

@samisayegh commented on 2020-07-08 21:51

Outdated location: line 28 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

Why is this line needed?

[coveobot]

@samisayegh commented on 2020-07-08 22:42

Outdated location: line 33 of packages/atomic/src/index.html

Hmm, I’m hesitant to accept this approach as the default way to add conditions. It’s on us to push for a better developer experience, because if we do not, no one else will. Many teams are counting on us: SF/Sitecore/ServiceNow integration teams, demo teams, PS, external partners etc.

Hundreds of people will have to implement the design decisions we make.

‌

How do these alternatives hold up?

1. Is it possible to pass functions as props to web components?

<script>
     function conditionForTemplateA(result) {
         return ...
     }

     function conditionForTemplateB(result) {
        return ...
     }
</script>

<atomic-result-template condition="conditionForTemplateA">...</atomic-result-template>
<atomic-result-template condition="conditionForTemplateB">...</atomic-result-template>

2. Allow developers to write javascript string conditions. We execute the strings inside an eval call. (we’ll need to check if SF allows eval calls. Not sure if they are blocked for security reasons).

<atomic-result-template condition="raw.fileType === 'pdf' || raw.fileType === 'word'">
// eval(condition)

3. Agree on string language for conditions (could be javascript syntax, or simpler), and write a parser for this language. e.g.

<atomic-result-template condition="raw.fileType is pdf|word, raw.author not TED]">
// const subconditions = condition.split(',');
// ...

‌

[coveobot]

@samisayegh commented on 2020-07-08 23:13

Outdated location: line 26 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

I want to follow up on the discussion in the last PR, of switching to a more “synchronous” registration approach. This would remove the need for the priority mechanic, needing to pass a layout/metadata to selectTemplate method (e.g. selectTemplate(result, ‘card’)), and a global template manager instance.

How does the current approach compare to having the ResultList component:

1. DOM query all result template atomic components contained in it. Templates will be evaluated from top to bottom.
2. Partition the templates according to the layout prop on the atomic result template components.

<atomic-result-list>
    <atomic-result-template layout="android"></<atomic-result-template>
    <atomic-result-template layout="iOS"></<atomic-result-template>
    <atomic-result-template layout="android"></<atomic-result-template>
    <atomic-result-template layout="iOS"></<atomic-result-template>
</atomic-result-list>

Inside AtomicResultList

private layoutTemplateMap: Record<layout: string, templates[{content, condition}]> = {};

componentDidLoad() {
    const allTemplates = document.querySelectorAll('atomic-result-list atomic-result-template');
    allTemplates.forEach(template => layoutTemplateMap[template.layout].push(template))
}

private get results() {
    const templates = layoutTemplateMap[this.layout];
    const manager = headlessTemplateManager(templates);
    const rendered = Mustache.render(manager.selectTemplate(result), result);

    return <div innerHTML={rendered}></div>
}

I see this solution as freeing up the headless logic from knowing about visual metadata info like layout. Seeing the templates ordered is in my opinion much easier to reason about than checking the priority attribute, and one less prop a dev needs to specify. Removing the global allows multiple result list instances that have separate template managers.

‌

What are your thoughts? @{5a3c40fad62dd77f94582df2} , @{557058:6fb10fac-dc5b-4d35-b648-4b2d42952452}

[coveobot]

@olamothe commented on 2020-07-09 12:37

Outdated location: line 33 of packages/atomic/src/index.html

The only thing you can pass as a web component prop is strings.

You can set properties using javascript, like JF did here, it’s just a JavaScript object after all.

#Using function directly

It’s not doable, unless we “eval them in the global scope”. Not a good idea. Super error/bug prone.

#Using eval

No, that’s a problem, and something that will be blocked by many different system. We should not go down that road. It is currently blocked by Salesforce, and most modern system will have CSP rules that blocks the use of eval, for good reasons.

#Using custom language

Seems overkill, error prone, hard to use etc.

For example, we have a custom language for query pipeline (QPL). At face value, it’s simple to understand (`boost foo by 45 when bar is bazz`), and it has created a ton of maintenance case for the Search API, spending time helping people understand the language, people not being able to do what they exactly need because not supported in the language etc.

‌

What I would see as “clean” is for us to implement something similar/inspired by React.createContext :

AKA: You have a function/class/object exposed by atomic that you use create templates condition function in, and you can pull from that context from inside the child component without having to explicitly wire every props by hand.

With a function you can do everything, and it’s easy to understand by any developer. We just can’t use eval because that’a a security liability.

[coveobot]

@olamothe commented on 2020-07-09 12:45

Outdated location: line 26 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

What you describe is similar to what we do in the JSUI, and it has caused many problem for Salesforce team, or for SPA type of application, where they want to have one search page in the application, but that behaves differently depending on some “context”.

In JSUI it’s even worse, since it’s registered in a static variable. But anyway…

They want to be able to inject/remove/modify templates dynamically, in a search page that already has been initialized.

Your approach would make it more static, it seems ?
Which could work for many cases, but would cause problem in other, relatively frequent, scenarios.

[coveobot]

@samisayegh commented on 2020-07-09 13:35

Outdated location: line 26 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

Your approach would make it more static, it seems ?

Yep, the idea is the presentation layer/the user of headless takes care of grouping the templates according to some context (layout, user privilege, plan entitlements, other), versus “asynchronously” registering the template with us and passing a bunch of metadata (e.g. priority, layout, etc.), and then needing pass the active metadata (e.g. the active layout, the user privilege, the entitlement) when calling selectTemplate.

With this approach, headless takes care of just the business logic of matching conditions against the result, and the order of execution. The presentation layer takes care of gathering the templates (async or sync) that should be used for the current context.

[coveobot]

@ThibodeauJF commented on 2020-07-15 18:51

Location: line 70 of packages/atomic/src/components/atomic-result-list/atomic-result-list.tsx

unique ID will be necessary when adding template components.

Mustache escapes variables:

All variables are HTML escaped by default. If you want to return unescaped HTML, use the triple mustache: {{{name}}}.

‌

[coveobot]

@ThibodeauJF commented on 2020-07-15 18:54

Outdated location: line 28 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

It justs removes the content from the dom so it’s not visible by accident after its registered.

[coveobot]

@ThibodeauJF commented on 2020-07-15 19:42

Outdated location: line 26 of packages/atomic/src/components/atomic-result-template/atomic-result-template.tsx

I’m still not convinced of the advantages of that approach. Losing dynamism and all.

As for layout, since we don’t have that concept yet, it’s not added, but it will be for sure kept inside Atomic, using Stencil’s state. We decided against the layout inside headless previously.

If we go this way:

Default result template would have a priority of 0, any user defined template would have a default priority of 1. The optional priority prop would be there for additional control.

Would it happen that often to have 2 templates fulfill the same conditions and default to using order/priority? Would customers use this often?