[DRAFT] Search components integration

[FranckyC]

PR Type

Feature

Description of the changes

This is a first non optimized version just to assess the technical feasibility. Not intended to be used as is or integrated elsewhere.

Hi @gavinbarron, @sebastienlevert , as discussed, here is a first functional version of search components integration:

• Search results
• Search filters
• Search verticals

There is still lot of work to do but, at a glance, here are the following changes I've made to make it work:

Tailwind

Added Tailwind CSS integration with postcss and JIT mode. This allow components to use Tailwind styles on demand through dynamically generated tailwind-styles.css file. Just use yarn start as usual and see the content of the file to updated automatically when you add new classes on your components.

This relies on a new tailwindcss task on @mgt project generating a tailwind-styles-css.ts file and imported into components on-demand:

"tailwindcss": "tailwindcss --postcss -i ../mgt-components/src/styles/tailwind-styles.module.ts -o ../mgt-components/src/styles/tailwind-styles-css.ts --minify",
"tailwindcss:watch": "tailwindcss --postcss -i ../mgt-components/src/styles/tailwind-styles.module.ts -o ../mgt-components/src/styles/tailwind-styles-css.ts --watch --minify"

import { styles } from './mgt-search-verticals-css';
import { styles as tailwindStyles } from '../../styles/tailwind-styles-css';
...
static get styles() {
  return [
    styles,
    tailwindStyles
  ];
}
...
public render() {
  return html`
      <div class="px-2.5">
        <div class="max-w-7xl ml-auto mr-auto mb-8">
        ...

Dynamic imports

For search filters and performances purpose, I need to load correct locale according to the current language using daysJs library. However, because the current solution is not configured to have multiple bundles, I need to add the inlineDynamicImports flag on the Rollup config to keep my code as is (only on es6 config for now).

As discussed, this is something that needs to be considered in the future to optimize distribution and performances.

Tests

Added test example for search verticals component with @open-wc/testing package, web test runner with esplugin/playwright.
Added also useDefineForClassFields on a custom tsconfig.test.json to make it work with lit (I'm not using Babel).

To run tests and watch mode just use yarn wtr:watch and the VSCode debug configuration "Tests debug (Edge)":

Localization

In our company scenario, localization works for both components internal labels but also settings via attributes (ex: vertical name in multiple languages). For setting values, we define the ILocalizedString interface relying on a mandatory "language" property in localization strings:

export const strings = { 
    language: "en-us",
    ....

{
        "key":"tab1",
        "tabName": {
            "default":"Tab 1",
            "fr-fr":"Onglet 1"
        },
        ...

Normally, it is linked to a language provider component globally available on the page and setting this property using the LocalizationHelper but I didn't include it in this demo but you get the idea. Maybe something to think about in the future.

Next steps

Before going forward, we need to decide how to:

• Integrate these components in MGT. As discussed, it seems to be fairly easy to make them work in the existing MGT stack. However it brings a lot of search dedicated classes not useful for other components, increasing the bundle size for nothing. IMO, this should be either a project on its own in the monorepo structure with its own bundle (like mgt-search, why not with its own stack like Webpack :D) or a separate repo. For the latter, this will probably fall under the microsoft-search organization as the new PnP Modern Search including both components and SPFx webparts).
• Merge features from MGT existing search components and upcoming ones.
• Complete integration to adapt with existing MGT features (caching, styles, doc, etc.)

PR checklist

• Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
• All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
• Stories have been added and existing stories have been tested
• Added appropriate documentation. Docs PR:
• License header has been added to all new source files (yarn setLicense)
• Contains NO breaking changes

Other information

[ghost]

Thank you for creating a Pull Request @FranckyC.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

• I have verified a documentation PR has been linked and is approved (or not applicable)
• I have ran this PR locally and have tested the fix/feature
• I have verified that stories have been added to storybook (or not applicable)
• I have tested existing stories in storybook to verify no regression has occured
• I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

[FranckyC]

@gavinbarron @sebastienlevert did you have a chance to review this one and see how we can integrate? Thanks!

[sebastienlevert]

@FranckyC with v3.0 and vacations, this was postponed. We have planed time next week to review the PR and understand what it means for the search components! We'll keep you posted and comment as part of this PR. Thanks!

[sebastienlevert]

Hi @FranckyC, sorry it's taken so long to get back to you on this.

There is a lot here! Awesome work!
We had a few struggles to get the project building until we realized that the way you have things working is to be in the packages/mgt folder and running scripts from there. In most cases we set up our scripts to be run from the root directory.

Improved localization support

We love this, it's a great feature that will allow us to provide support for multilingual sites. Ideally we'd like to bring this into the library as a separate feature/PR which can be built on top of for other aspects of this change.

Based on what we can see it's a little unclear how the language that the user is viewing the component in is set. It would be good to provide some more clarity around that.

Dynamic imports

We'll need to explore this more thoroughly as we also need to ensure that the dynamic imports work as expected when used from the @microsoft/mgt-components library.

Although I do wonder if we can achieve the same outcomes as dayJS simply using the browser built in APIs as they do provide great i18n support for language that are installed in the browser. We have an existing example of using the Intl date APIs here: https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/main/packages/mgt-components/src/components/mgt-agenda/mgt-agenda.ts#L667

Is rendering a date using the French output something we'd expect to be doing when the French language is not installed in the browser?

I see that the Adaptive cards work is also attempting to use dynamic imports. Dynamic imports are well understood by the TypeScript compiler, we'll just need to adjust the bundler being used to build the bundled output for @microsoft/mgt

Tests

😍 Thank you so much for providing us with an example of using the web test runner from @open-wc/testing! We burnt a fair bit of time trying to get this running last year, gave up, and went back to Jest as the team was more familiar with it. We love difference in performance here. We'll be going to spend some time looking into coverage reporting and how to combine this with traditional unit tests but we suspect we'll be taking this work on and moving away from Jest.

Tailwind

This one is tricky, it's such a shift from how we style all our other web components and is very challenging to adopt due to our investment in @fluentui/web-components and that styling/theming system. We don't think that this is a dependency that we can add to MGT.
We did some brainstorming about pathways forward and concluded that with some refactoring of the search components to ensure that each fragment of html rendering was placed in a protected method then it should be feasible to share all of the logic for assembling the component, data acquisition, etc. From there other implementers can extend the MgtSearch* classes and provide their own render methods and styles similar to the agenda extensions example

Next steps.

Component integration

You raise a valid point about the additional classes and code weight that this brings to the packages, that said we're not overly concerned, as when folks are building using a modern bundling tool the unsused components from @microsoft/mgt-components will be shaken out for production builds leaving only the code that the developer needs to have. (See #2642)

In terms of the @microsoft/mgt package if folks are using es6 and not explicitly using the bundled packages they get the same behavior as if they were using @microsoft/mgt-components directly. We're currently shipping the big bundles to provide the an easy to use, if slow to load, single point of entry when building in HTML without a bundler. This is a use case we need to get more data on as we firmly believe that 95%+ of our developer audience will be using a modern tool chain and bundler.

If we do see real value in spinning off a separate @microsoft/mgt-search-components package in the future that's a relatively straightforward refactor, the only complication would be around our React shim package that we auto-generate.

Let's target these changes sitting inside the microsoft/mgt-components package for now.

Localization improvments

If this can be split out into a separate change with additional examples to illustrate usage patterns that would be amazing and likely the first of these changes to be brought into the main branch.

Again, THANK YOU! This is really fantastic work.

p.s. It occurs to us that making you a contributor in the repo and moving your branch there might allow us to collaborate more on this code with the team and we'll be able to make PR's against the feature or WIP branch and also allowing our automation to perform test deployments. Is this a way of working that you would be comfortable with?

[FranckyC]

Thanks @sebastienlevert

Here are my comments regarding each point:

Improved localization support

We love this, it's a great feature that will allow us to provide support for multilingual sites. Ideally we'd like to bring this into the library as a separate feature/PR which can be built on top of for other aspects of this change.

Based on what we can see it's a little unclear how the language that the user is viewing the component in is set. It would be good to provide some more clarity around that.

The language is determined by the localization files locale. I didn't add the language provider component but basically it does the following:

// Load locale file dynamically base on selected locale (this comes from the component options or env)
const localizedResources = await import(
	/* webpackChunkName: "chunk-[request]" */
	/* webpackExports: ["strings"] */
	`../../loc/strings.${locale}.ts`
);

if (localizedResources) {
	LocalizationHelper.strings = localizedResources.strings;
}

Then every component always loads the default locale file strings.default and implements the strings() accessor:

import { MgtSearchResultsStrings as strings } from './loc/strings.default';

protected get strings() {
	return strings;
}

Whenever the LocalizationHelper.strings property is updated, components get re-rendered with getting the new value for strings corresponding to that component in the localization file. So:

strings.defaults.ts looks like this:

export const strings = {
  language: 'en-us',
  _components: {
    'mgt-search-results': { ...MgtSearchResultsStrings },
	...

And other localization files (ex strings.fr-fr.ts):

export const strings = { 
    language: "fr-fr",
    _components: {
        "mgt-search-results": {
            seeAllLink: "Voir tout",
            results: "résultats"
        },
		...

Dynamic imports

We'll need to explore this more thoroughly as we also need to ensure that the dynamic imports work as expected when used from the @microsoft/mgt-components library.

Although I do wonder if we can achieve the same outcomes as dayJS simply using the browser built in APIs as they do provide great i18n support for language that are installed in the browser. We have an existing example of using the Intl date APIs here: https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/main/packages/mgt-components/src/components/mgt-agenda/mgt-agenda.ts#L667

Is rendering a date using the French output something we'd expect to be doing when the French language is not installed in the browser?

I used daysJs mainly because it mimics moment.js syntax and since I reused my code for PnP Modern Search it was way easier :D. The library is quite small so I believe this is not really an issue. Probably it could be replaced by native i18n features but I didn't test quite frankly.

The thing is this is not only making the TypeScript compiler happy by using inlineDynamicImports: true :D Having dynamic imports in a monolithic bundle sounds like an anti pattern. Having dynamic imports means having multiple bundles loaded on-demand to improve performances. Adaptive card is a good example but we have other use cases where dynamic import is useful as well.

By introducing dynamic imports in MGT means to host multiple files instead of one big bundle currently so deployment process would need to be updated as well.

Tests

😍 Thank you so much for providing us with an example of using the web test runner from @open-wc/testing! We burnt a fair bit of time trying to get this running last year, gave up, and went back to Jest as the team was more familiar with it. We love difference in performance here. We'll be going to spend some time looking into coverage reporting and how to combine this with traditional unit tests but we suspect we'll be taking this work on and moving away from Jest.

These tests are not really meant for unit testing but more for end-to-end tests. In the case of web components like this, this is way more useful :D

Tailwind

This one is tricky, it's such a shift from how we style all our other web components and is very challenging to adopt due to our investment in @fluentui/web-components and that styling/theming system. We don't think that this is a dependency that we can add to MGT.

I think both approaches can be complementary. Using Tailwind has no impact on the "consumption" side as styles are not exposed in templates anyway. It is only at build time and we only expose CSS variables to consumers. There is probably some work to do to align variables from fluentui theme and Tailwind but that should be feasible.

Having Tailwind or not regarding protected methods like renderLocation(), renderAttendees() does not really matter here actually. It is up to developer extending the component to use the styling system he wants, just like I did for search components by extending MgtTemplatedComponent.

We did some brainstorming about pathways forward and concluded that with some refactoring of the search components to ensure that each fragment of html rendering was placed in a protected method then it should be feasible to share all of the logic for assembling the component, data acquisition, etc. From there other implementers can extend the MgtSearch* classes and provide their own render methods and styles similar to the agenda extensions example

You mean the protected methods like renderLocation(), renderAttendees() etc.? Having Tailwind or not does not really matter here actually. It is up to developer extending the component to use the styling system he wants, just like I did for search components by extending MgtTemplatedComponent.

Next steps.

Component integration

You raise a valid point about the additional classes and code weight that this brings to the packages, that said we're not overly concerned, as when folks are building using a modern bundling tool the unsused components from @microsoft/mgt-components will be shaken out for production builds leaving only the code that the developer needs to have. (See #2642)

In terms of the @microsoft/mgt package if folks are using es6 and not explicitly using the bundled packages they get the same behavior as if they were using @microsoft/mgt-components directly. We're currently shipping the big bundles to provide the an easy to use, if slow to load, single point of entry when building in HTML without a bundler. This is a use case we need to get more data on as we firmly believe that 95%+ of our developer audience will be using a modern tool chain and bundler.

Yes, my main concern was about the size of this big JavaScript bundle but if you think this is not the way developers consume MGT, that's fine :D Having dynamic imports could help there for sure.

If we do see real value in spinning off a separate @microsoft/mgt-search-components package in the future that's a relatively straightforward refactor, the only complication would be around our React shim package that we auto-generate.

Let's target these changes sitting inside the microsoft/mgt-components package for now.
Localization improvments

If this can be split out into a separate change with additional examples to illustrate usage patterns that would be amazing and likely the first of these changes to be brought into the main branch.

Again, THANK YOU! This is really fantastic work.

p.s. It occurs to us that making you a contributor in the repo and moving your branch there might allow us to collaborate more on this code with the team and we'll be able to make PR's against the feature or WIP branch and also allowing our automation to perform test deployments. Is this a way of working that you would be comfortable with?

Yes sure it will be easier I guess.

[sebastienlevert]

You mean the protected methods like renderLocation(), renderAttendees() etc.? Having Tailwind or not does not really matter here actually. It is up to developer extending the component to use the styling system he wants, just like I did for search components by extending MgtTemplatedComponent.

We think that a developer that wants to bring their own ways of styling would be better inheriting from the "base" Search components (which would be a native MGT component without Tailwind) and add the styling pieces in the render* methods. That way, we don't add a dependency that adds maintenance to the global package, while leaving opportunities to the developer to extend it the way they want. Does it make more sense? Basically, MGT would have a Fluent version but every method would be overridable and could leverage any CSS framework.

Yes, my main concern was about the size of this big JavaScript bundle but if you think this is not the way developers consume MGT, that's fine :D Having dynamic imports could help there for sure.

We have a PR up for making proper bundling and better treeshaking. I think this will absolutely help and should reduce a lot the size of projects when leveraging just some components. #2642

As soon as @gavinbarron is back from holidays we'll add your access to our team and we will be able to continue the discussion! Thanks!

[sonarcloud]

Quality Gate failed

Failed conditions
9 Security Hotspots
D Security Rating on New Code (required ≥ A)
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.