Implement Typescript and ESM, and add Parcel for builds #27
Conversation
|
Currently, there are no developers on the Yivi project. Therefore, this code will not be reviewed anywhere soon. It is not clear yet how this will look like. |
| _stateMachine: any; | ||
| _options: any; | ||
| _session: any; | ||
| _onCancel: any; |
There was a problem hiding this comment.
Since these properties are already marked as private with an underscore, it might be nice to enforce it with the TypeScript private keyword.
| @@ -28,7 +38,7 @@ module.exports = class DOMManipulations { | |||
| if (state.isFinal) { | |||
| this._detachEventHandlers(); | |||
| // Make sure all restart buttons are hidden when being in a final state | |||
| this._element.querySelectorAll('.yivi-web-restart-button').forEach((e) => (e.style.display = 'none')); | |||
| this._element.querySelectorAll('.yivi-web-restart-button').forEach((e) => ((e as HTMLButtonElement).style.display = 'none')); | |||
There was a problem hiding this comment.
The querySelectorAll method is generic, so we can specify the type of the elements like so:
| this._element.querySelectorAll('.yivi-web-restart-button').forEach((e) => ((e as HTMLButtonElement).style.display = 'none')); | |
| this._element.querySelectorAll<HTMLButtonElement>('.yivi-web-restart-button').forEach((e) => (e.style.display = 'none')); |
| @@ -237,21 +248,21 @@ module.exports = class DOMManipulations { | |||
| } | |||
|
|
|||
| _stateEnterPairingCode({ transition, payload }) { | |||
| const form = this._element.querySelector('.yivi-web-pairing-form'); | |||
| const form = this._element.querySelector('.yivi-web-pairing-form') as HTMLFormElement; | |||
There was a problem hiding this comment.
The querySelector method is also generic, but in this case it might be more a matter of preference of how to write it.
| _resolve: any; | ||
| _reject: any; | ||
|
|
||
| constructor(options: Record<string, any>) { |
There was a problem hiding this comment.
Oh cool! I never knew this type existed. 😅 I always use something like { [key: string]: any }, but the Record type seems way better :)
(This comment can be resolved. It's just something I found interesting.)
Closes #15
What is changed
I've implemented a typescript for each package except
yivi-console,yivi-dummyandyivi-css.Typescript is implemented with the weakest setting
strict: falsein thetsconfig.json, meaning that there is almost no "type safety", but that can be improved later to keep this PR as minimal as possible.Typescript with
strict: falseis almost the same as ESM.To support different environments, like NodeJS and bundlers like Vite a bundler is needed to output script for each environment. I've added Parcel, which is a super simple bundler, that uses the
package.jsonentry paths (module,main,browser,source, etc) to configure the bundle configuration.In
yivi-frontendI've replaced the package imports with relative paths since the bundler doesn't like "external" packages to be built in, It is possible but costs some effort to make that work. And since these packages are "DEV" only and bundled in using relative paths, they are bit easier. An alternative strategy is to make them "external", but then it is required to mark them as "dependencies" inpackage.json.Future improvements
NPM workspaces
I would recommend switching this repo to NPM Workspaces, which is designed and made for frontend mono repo. For example, makes installing the package much easier, for example running
npm installfrom the root.Typescript typing
After this PR there are a lot of types of "any" which is not benefit from the power of typescript. Making things strictly typed will help prevent bugs in the future
Testing
I would recommend starting with writing some tests for these packages to make sure everything is working correctly in the future. You may not realizing that this library will grow in the coming years, since Yivi is getting some attention lately.