Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(headless): add case-assist sub package #717

Merged
merged 17 commits into from
Apr 23, 2021
Merged

chore(headless): add case-assist sub package #717

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
34e1a86
add case-assist sub-module
samisayegh Apr 9, 2021
d9000bf
merge master
samisayegh Apr 14, 2021
7588462
add modules folder
samisayegh Apr 14, 2021
d291402
refactor rollup to process multiple modules
samisayegh Apr 15, 2021
ade2af1
place modules in pkg folder
samisayegh Apr 15, 2021
42d06fe
add contributors guide
samisayegh Apr 15, 2021
44ac73c
fix typedefinition imports
samisayegh Apr 15, 2021
09ef612
fix dev config
samisayegh Apr 15, 2021
cbedcb7
update definition references
samisayegh Apr 15, 2021
710b5b7
fix definitions
samisayegh Apr 15, 2021
8ddee64
undo fix
samisayegh Apr 15, 2021
58e576c
remove version field
samisayegh Apr 19, 2021
4dcf70c
rename case-assist entry file
samisayegh Apr 19, 2021
3a321f2
readd dummy export
samisayegh Apr 19, 2021
c43fdc9
merge master
samisayegh Apr 22, 2021
05c1089
place sub-package at root level; update guide
samisayegh Apr 22, 2021
9ad3d86
update import path
samisayegh Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/atomic/cypress/utils/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {Interception} from 'cypress/types/net-stubbing';
import {Result} from '../../../headless/dist/api/search/search/result';
import {SearchRequest} from '../../../headless/dist/api/search/search/search-request';
import {SearchResponseSuccess} from '../../../headless/dist/api/search/search/search-response';
import {Result} from '../../../headless/dist/definitions/api/search/search/result';
import {SearchRequest} from '../../../headless/dist/definitions/api/search/search/search-request';
import {SearchResponseSuccess} from '../../../headless/dist/definitions/api/search/search/search-response';
import {searchEndpoint} from './setupComponent';

function getApiCall(selector: string): Promise<Interception> {
Expand Down
4 changes: 2 additions & 2 deletions packages/atomic/src/components/atomic-breadcrumb-manager/atomic-breadcrumb-manager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import {
Breadcrumb,
BreadcrumbValue,
} from '@coveo/headless';
import {RangeFacetValue} from '@coveo/headless/dist/features/facets/range-facets/generic/interfaces/range-facet';
import {BaseFacetValue} from '@coveo/headless/dist/features/facets/facet-api/response';
import {RangeFacetValue} from '@coveo/headless/dist/definitions/features/facets/range-facets/generic/interfaces/range-facet';
import {BaseFacetValue} from '@coveo/headless/dist/definitions/features/facets/facet-api/response';
import mainclear from '../../images/main-clear.svg';
import dayjs from 'dayjs';

Expand Down
36 changes: 36 additions & 0 deletions packages/headless/contributors/adding-a-sub-package.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Adding a sub-package
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preview: https://github.com/coveo/ui-kit/blob/KIT-605/packages/headless/contributors/adding-a-sub-package.md


Headless provides exports through multiple sub packages. A sub-package groups together exports (i.e. controllers, actions, reducers, engines) that work together as a cohesive unit. By separating exports into sub-packages, it becomes clear to users of headless what exports are available to build a use-case.


## To add a new sub-package:

1. Create an entry file for your sub-package inside the `src` directory (e.g. case-assist.ts).
2. Configure nodejs and browser bundles inside `rollup.config.js` for your entry file.
3. Inside the `pkg` directory, create a new directory having the name of your sub-package.
4. Inside the new directory, add a `package.json` file and fill in the paths to your bundled files and type definitions.

```json
pkg/case-assist/package.json

{
"name": "case-assist",
"version": "1.0.0",
"description": "Headless Case Assist Module",
"main": "../../dist/case-assist/headless.js",
"module": "../../dist/case-assist/headless.esm.js",
"browser": "../../dist/browser/case-assist/headless.esm.js",
"types": "../../dist/definitions/case-assist.d.ts",
"license": "Apache-2.0"
}
```

## Testing your sub-package:

1. Build the headless project: `npm run build`.
2. Create a tarball: `npm pack`.
3. Install the tarball as a dependency of a different project: `npm i <path to the tarball>`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 ?

Why pack ? npm link does not work in this case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commands are similar, but I think npm pack is more accurate because it creates an output matching what is uploaded npm.

For example, this config in the package.json tells npm what files to include in the tarball. Say someone created a new directory, but did not update the config. I suspect a local symlink would provide access to the new directory, but when installing from npm, the directory would be missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

I just know that personally, I would hate to work with that flow and would not do as described here :D

Seems like a hassle for "maybe it's going to be more accurate".

4. Import an export from your sub-package: `import {...} from '@coveo/headless/pkg/<sub-package>'`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway we can do it without the /pkg/ in there ? it's kind of useless info, since it's always going to be there for all sub packages

ie:

import { buildCaseDeflection } from '@coveo/headless/case-assist

Copy link
Contributor Author

@samisayegh samisayegh Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be possible, but with one draw-back: the case-assist directory holding the package.json file needs to be at the root level.

My concern was if headless has n packages, then the root level will become cluttered:

dist/
src/
package1/
package2/
package3/
...

By grouping the sub-packages under a pkg dir, the root is more tidy. Personally, I prefer the shorter path same as you, but I did this as a compromise.

I will check if it is possible to specify a base-path when creating the tarball, but assuming it is not, what do you think between adding the sub-packages to the root vs. grouping them under a dir?

Copy link
Contributor Author

@samisayegh samisayegh Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates:

Node 12.7 added support for an exports field to allow mapping one path to another.

However, this does not yet work inside typescript projects. Support is expected as part of TS 4.3 due next month, but it is still not working on the beta.

Maybe we could start with top-level directories, and refactor once support is available. This would provide our ideal api, and avoid a breaking change.

I will look further tonight and make the decision tomorrow,



That's all!
7 changes: 4 additions & 3 deletions packages/headless/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,18 @@
"./dist/headless.js": "./dist/browser/headless.js",
"./dist/headless.esm.js": "./dist/browser/headless.esm.js"
},
"types": "./dist/index.d.ts",
"types": "./dist/definitions/index.d.ts",
"license": "Apache-2.0",
"version": "0.10.0",
"files": [
"dist/"
"dist/",
"pkg/"
],
"scripts": {
"start": "concurrently \"npm run typedefinitions -- -w\" \"rollup -c -w\"",
"build": "npm run clean && npm run build:prod",
"build:prod": "npm run typedefinitions && rollup -c --environment BUILD:production",
"typedefinitions": "tsc -p src/tsconfig.build.json -d --emitDeclarationOnly --declarationDir dist",
"typedefinitions": "tsc -p src/tsconfig.build.json -d --emitDeclarationOnly --declarationDir dist/definitions",
samisayegh marked this conversation as resolved.
Show resolved Hide resolved
"clean": "rimraf -f -r dist/*",
"test": "jest",
"test:watch": "jest --watch --colors --no-cache --silent=false",
Expand Down
10 changes: 10 additions & 0 deletions packages/headless/pkg/case-assist/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
samisayegh marked this conversation as resolved.
Show resolved Hide resolved
"name": "case-assist",
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Euhmmm... how will that work exactly with lerna + semantic commit ?

What does that "version" represent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it, it's a result of running npm init to generate this file.

This package.json is just a way to tell IDEs and bundlers where the js and typedefinitions are located for a sub-package. The source of truth for the version published to npm is the one in the headless root package.json

"description": "Headless Case Assist Module",
"main": "../../dist/case-assist/headless.js",
"module": "../../dist/case-assist/headless.esm.js",
"browser": "../../dist/browser/case-assist/headless.esm.js",
"types": "../../dist/definitions/case-assist.d.ts",
"license": "Apache-2.0"
}
179 changes: 115 additions & 64 deletions packages/headless/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,81 +36,132 @@ function onWarn(warning, warn) {
warn(warning);
}

const nodeConfig = {
input: 'src/index.ts',
output: [
{file: 'dist/headless.js', format: 'cjs'},
{file: 'dist/headless.esm.js', format: 'es'},
],
plugins: [
resolve({modulesOnly: true}),
commonjs({
// https://github.com/pinojs/pino/issues/688
ignore: ['pino-pretty'],
}),
typescript(),
replace(),
],
external: ['cross-fetch', 'web-encoding'],
onwarn: onWarn,
};

const browserConfig = {

// Node Bundles

const nodejs = [
{
input: 'src/index.ts',
outDir: 'dist',
},
{
input: 'src/case-assist.ts',
outDir: 'dist/case-assist'
}
].map(buildNodeConfiguration);

function buildNodeConfiguration({input, outDir}) {
return {
input,
output: [
{file: `${outDir}/headless.js`, format: 'cjs'},
{file: `${outDir}/headless.esm.js`, format: 'es'},
],
plugins: [
resolve({modulesOnly: true}),
commonjs({
// https://github.com/pinojs/pino/issues/688
ignore: ['pino-pretty'],
}),
typescript(),
replace(),
],
external: ['cross-fetch', 'web-encoding'],
onwarn: onWarn,
};
}


// Browser Bundles

const browser = [
{
input: 'src/index.ts',
output: [
buildUmdOutput('dist/browser', 'CoveoHeadless'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any generated documentation that lists the packages and their content?
If yes, where is it?
If not, is that something we plan on providing in the short term?

Just trying to think how customers will find out whether to use CoveoHeadless or CoveoHeadlessCaseAssist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation does not display the new case-assist module at the moment, but it's something we can address in the short-term. We would add a section on how to consume headless via CDN and can include the name of the injected global.

buildEsmOutput('dist/browser')
]
},
{
input: 'src/case-assist.ts',
output: [
buildUmdOutput('dist/browser/case-assist', 'CoveoHeadlessCaseAssist'),
buildEsmOutput('dist/browser/case-assist')
]
}
].map(buildBrowserConfiguration);

function buildBrowserConfiguration({input, output}) {
return {
input,
output,
plugins: [
alias({
entries: [
{
find: 'coveo.analytics',
replacement: pathResolve(
__dirname,
'./node_modules/coveo.analytics/dist/library.es.js'
),
},
{
find: 'cross-fetch',
replacement: pathResolve(__dirname, './fetch-ponyfill.js'),
},
{
find: 'web-encoding',
replacement: pathResolve(__dirname, './node_modules/web-encoding/src/lib.js'),
}
],
}),
resolve({browser: true}),
commonjs(),
typescript(),
replace(),
isProduction && sizeSnapshot(),
isProduction && terser(),
],
}
}

function buildUmdOutput(outDir, name) {
return {
file: `${outDir}/headless.js`,
format: 'umd',
name,
sourcemap: isProduction
}
}

function buildEsmOutput(outDir) {
return {
file: `${outDir}/headless.esm.js`,
format: 'es',
sourcemap: isProduction,
}
}



// For Atomic's development purposes only
const dev = buildBrowserConfiguration({
input: 'src/index.ts',
output: [
{
file: 'dist/browser/headless.js',
format: 'umd',
name: 'CoveoHeadless',
sourcemap: isProduction,
},
{
file: 'dist/browser/headless.esm.js',
format: 'es',
sourcemap: isProduction,
},
// For Atomic's development purposes only
{file: '../atomic/src/external-builds/headless.esm.js', format: 'es'},
],
plugins: [
alias({
entries: [
{
find: 'coveo.analytics',
replacement: pathResolve(
__dirname,
'./node_modules/coveo.analytics/dist/library.es.js'
),
},
{
find: 'cross-fetch',
replacement: pathResolve(__dirname, './fetch-ponyfill.js'),
},
{
find: 'web-encoding',
replacement: pathResolve(__dirname, './node_modules/web-encoding/src/lib.js'),
}
],
}),
resolve({browser: true}),
commonjs(),
typescript(),
replace(),
isProduction && sizeSnapshot(),
isProduction && terser(),
],
};
buildEsmOutput('../atomic/src/external-builds')
]
});


// Api-extractor cannot resolve import() types, so we use dts to create a file that api-extractor
// can consume. When the api-extractor limitation is resolved, this step will not be necessary.
// [https://github.com/microsoft/rushstack/issues/1050]
const typeDefinitions = {
input: "./dist/index.d.ts",
input: "./dist/definitions/index.d.ts",
output: [{file: "temp/headless.d.ts", format: "es"}],
plugins: [dts()]
}

const config = isProduction ? [nodeConfig, typeDefinitions, browserConfig] : [browserConfig];
const config = isProduction ? [...nodejs, typeDefinitions, ...browser] : [dev];

export default config;
3 changes: 3 additions & 0 deletions packages/headless/src/case-assist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function hello() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove I guess ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think it would be nicer if we had:

packages/headless/src/case-assist/index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will empty the file and keep just a comment for git.

For creating a case-assist directory, I feel this divides case-assist functionality into it's own branch of the tree, which I don't think we should do.

Currently it looks like this:

headless_directory_structure

I think we should keep controllers together, features together, and group functionality under those concepts, rather than by sub-package. Some controllers will be used by multiple packages after all.

controllers/
    controller1
    controller2

If the goal behind the directory is to have the word index in the file name though, we could call it:

case-assist.index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: readding a dummy export because rollup throws an error otherwise.

return 'hello world';
}
8 changes: 4 additions & 4 deletions packages/quantic/coveo.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/* eslint-disable node/no-unpublished-import */
import * as HeadlessTypes from './force-app/main/default/staticresources/coveoheadless/index';
export * from './force-app/main/default/staticresources/coveoheadless/index';
import * as HeadlessTypes from './force-app/main/default/staticresources/coveoheadless/definitions/index';
export * from './force-app/main/default/staticresources/coveoheadless/definitions/index';
import {LightningElement} from 'lwc';
import {HeadlessEngine} from '../headless/dist/index';

declare global {
// eslint-disable-next-line no-undef
const CoveoHeadless: typeof HeadlessTypes;
Expand All @@ -15,7 +15,7 @@ declare global {
initialized: boolean;
}[];
config: HeadlessConfigurationOptions;
engine: HeadlessEngine;
engine: HeadlessTypes.HeadlessEngine;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Maybe I'm just confused, but will that work when passing an engine created through a sub-package? I know that both engines will use the same HeadlessEngine class, but since engines are exposed from two different packages, they will be two different types, no?

Copy link
Contributor Author

@samisayegh samisayegh Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, there is only one engine class so the type will be the same. I'm not sure if separate types are needed just yet. I will investigate further while looking into how to pass different api clients to different engine instances.

};
};
}
Expand Down