chore(headless): add case-assist sub package#717
Conversation
samisayegh
requested review from
ThibodeauJF,
btaillon-coveo and
olamothe
as code owners
| @@ -0,0 +1,36 @@ | |||
| # Adding a sub-package | |||
There was a problem hiding this comment.
|
Pull Request Report PR Title ✅ Title follows the conventional commit spec. Bundle Size
|
packages/headless/src/case-assist.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| export function hello() { | |||
There was a problem hiding this comment.
Also, I think it would be nicer if we had:
packages/headless/src/case-assist/index.ts
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
Edit: readding a dummy export because rollup throws an error otherwise.
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "name": "case-assist", | |||
| "version": "1.0.0", | |||
There was a problem hiding this comment.
Euhmmm... how will that work exactly with lerna + semantic commit ?
What does that "version" represent ?
There was a problem hiding this comment.
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
|
|
||
| 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>`. |
There was a problem hiding this comment.
🤔 ?
Why pack ? npm link does not work in this case ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
| 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>`. | ||
| 4. Import an export from your sub-package: `import {...} from '@coveo/headless/pkg/<sub-package>'` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Updates:
Node 12.7 added support for an exports field to allow mapping one path to another.
- https://stackoverflow.com/questions/51313753/npm-package-json-base-root-property?noredirect=1#comment89604253_51313753
- https://github.com/jkrems/proposal-pkg-exports/#example
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.
- Support for NodeJS 12.7+ package exports microsoft/TypeScript#33079 (comment)
- TypeScript 4.3 Iteration Plan microsoft/TypeScript#42762
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,
| { | ||
| input: 'src/index.ts', | ||
| output: [ | ||
| buildUmdOutput('dist/browser', 'CoveoHeadless'), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| }[]; | ||
| config: HeadlessConfigurationOptions; | ||
| engine: HeadlessEngine; | ||
| engine: HeadlessTypes.HeadlessEngine; |
There was a problem hiding this comment.
🤔 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?
There was a problem hiding this comment.
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.
4 participants
This PR refactors the rollup file to make it easy to add sub-packages to headless.