[BB pr#5] KIT-10 Init Atomic web-component package with stencil

[coveobot]

Pull request 🔀 created by @samisayegh on 2020-04-27 17:41
Last updated on 2020-04-29 13:14
Original Bitbucket pull request id: 5

Participants:

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

Source: Commit a9365ab82a3a on branch KIT-10> Destination: https://bitbucket.org/coveord/ui-kit/commits/0c40bc1d43c1 on branch master
Merge commit: https://github.com/None/commit/a72ae9100afa

State: MERGED

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

‌

[coveobot]

@samisayegh commented on 2020-04-27 17:43

I initialized the package using stencil with very few modifications to make this initial PR easier to review. I will leave comments on things I changed. I kept the example component as a guide for our first component, after which we can remove the example.

[coveobot]

@samisayegh commented on 2020-04-27 17:45

Location: line 10 of .eslintrc.json

I specified the environment so that the linting step when committing could handle jest test files.

[coveobot]

@samisayegh commented on 2020-04-27 17:47

Location: line 201 of packages/atomic/LICENSE

I changed the license from MIT to apache 2.0. Note that we still technical need to add a comment at the top of files. I will look at this as part of the bundler configuration.

[coveobot]

@samisayegh commented on 2020-04-27 17:49

Outdated location: line 65 of packages/atomic/readme.md

I adjusted the README.md to introduce the project, and reduce the Stencil-specific text. I left the unpkg cdn example on line 54 for now. I was thinking of revising this at a later step in a separate PR.

[coveobot]

@samisayegh commented on 2020-04-27 18:13

Location: line 2 of packages/atomic/tsconfig.json

I adjusted this file to extend from the base tsconfig.json.

[coveobot]

@samisayegh commented on 2020-04-27 18:25

Location: line 28 of packages/atomic/package.json

Updated the name, description and license properties. Note that we cannot release a package with the @coveo scope until we have an org with the same name. I opened a request to create it with IT.

[coveobot]

@olamothe commented on 2020-04-27 18:26

Location: line 6 of packages/atomic/package-lock.json

Some (most?) of those should actually be dev dependencies.

It’s very helpful for snyk scan.

[coveobot]

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

Outdated location: line 9 of packages/atomic/readme.md

I don’t think we need this, it’s all arbitrary :slight_smile:

I’d cut straight to the point about what you need to start and run/compile the project.

You could link to generic online doc about web components/stencil doc to help readers, I guess.

[coveobot]

@olamothe commented on 2020-04-27 18:32

Outdated location: line 13 of packages/atomic/readme.md

I’m wondering if instead we should not put all of that in top level repo README ? Since, normally, a dev would want to run everything with lerna for best dev experience, versus each sub-packages individually.

[coveobot]

@samisayegh commented on 2020-04-27 18:34

Location: line 6 of packages/atomic/package-lock.json

Yep, I saved the two dependencies as devDependencies in the package.json, but in the package-lock.json, everything is kept under the dependencies key.

[coveobot]

@olamothe commented on 2020-04-27 18:35

Outdated location: line 5 of packages/atomic/tsconfig.json

Why is this needed here ? I don’t think it’s allowed in the base tsconfig either.

[coveobot]

@olamothe commented on 2020-04-27 18:35

Location: line 5 of packages/atomic/tsconfig.json

Why does it need to be false ?

[coveobot]

@olamothe commented on 2020-04-27 18:36

Location: line 16 of packages/atomic/tsconfig.json

Curious, how does changing the jsx implementation affect the bundle size ? Since all components will use that extensively, it’s something that might be worthwhile to optimize early.

[coveobot]

@olamothe approved ✔️ the pull request on 2020-04-27 18:37

[coveobot]

@ThibodeauJF approved ✔️ the pull request on 2020-04-27 19:34

[coveobot]

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

Outdated location: line 13 of packages/atomic/readme.md

Looking at the react-vapor repo, they have a README.md at the top-level, as well as for every package.

Commands common to all packages, when run from the root, will run for every package. This is the case for npm run build which generates the files to be distributed to production. I will document it at the top level.

I think having package-specific commands at the package-level README.md along with some duplication is also good to focus on a particular package. That said, I will remove the installation step and link to the root level README instead.

[coveobot]

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

Location: line 16 of packages/atomic/tsconfig.json

For jsx, we are using Stencil’s library called h specified on the next line. It needs to be imported into all files using jsx syntax. I will keep in mind to look into performance metrics, but to start, we are using the implementation offered by the Stencil framework.

[coveobot]

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

Outdated location: line 5 of packages/atomic/tsconfig.json

Indeed, I removed it.

[coveobot]

@samisayegh commented on 2020-04-28 22:09

Location: line 5 of packages/atomic/tsconfig.json

I’m not sure atm. The values in this file are those generated when initializing the project. They differ in many cases from those in the gts dependency. For example, I’m not sure why module is set to esnext versus the commonjs in gts, or why target is es2017 and not es2018 like in gts.

‌

For declarations, I see there is a file in the src dir called component.d.ts with the following header comment:

/**
 * This is an autogenerated file created by the Stencil compiler.
 * It contains typing information for all components that exist in this project.
 */

‌

Interestingly, looking at the types property in the package.json, it’s not this file that is specified but rather an index.d.ts which references the above file (maybe to allow adding type defs from other libraries?)

I am trusting Stencil’s tooling blindly on this initial commit. I will delve into why it is doing things this way when we are closer to publishing the first version, but for now I prefer to stick to the defaults they have specified.

[coveobot]

@btaillon commented on 2020-04-29 12:03

Location: line 5 of packages/atomic/tsconfig.json

@{557058:ef2198d5-03b4-4d5f-86e7-0a23e51f4f14} I think this is to scope what declarations are available to the public. By creating our own .d.ts files such as in component.d.ts, we can choose what types are exposed to importing packages.

[coveobot]

@btaillon approved ✔️ the pull request on 2020-04-29 12:05