Skip to content

TypeScript + Monorepo + Various fixes#144

Open
MarcoScabbiolo wants to merge 13 commits intoSamVerschueren:masterfrom
MarcoScabbiolo:master
Open

TypeScript + Monorepo + Various fixes#144
MarcoScabbiolo wants to merge 13 commits intoSamVerschueren:masterfrom
MarcoScabbiolo:master

Conversation

@MarcoScabbiolo
Copy link

Based on the conversation at #143

This PR is a complete rewrite in TypeScript using lerna to include the 3 main renderers and jest/babel for testing. It also introduces a new package, listr-core which has the classes and types shared between Listr and a renderer.

The build results contain the type definitions, so there will be no need for @types/listr anymore.

One of the main issues while using listr was its dependency on RxJS, that has been removed and replaced by a barebone Observable + Observer implementation to preserve the spirit of the base code as best as possible while also minimising changes to the Task-Renderer interface.

Some bugs/odd behaviours have been fixed and they will affect end users and should be considered breaking changes, although they were strange and buggy behaviours. You can see these by looking at the differences between the old tests and the new ones

There are also breaking changes in the Task - Renderer interface, so any custom renderer out there will have to update to the new interface and inherit from BaseRenderer in list-core.

Code coverage is not reflected properly as listr-core is mostly tested via tests in listr that use the builded .js files and not the actual source code, so the metric itself is not representative right now. All the tests have been preserved and even some additional checks have been added.

i've set lerna in singl-version mode bumping all packages to 0.15.0, but that is of course subject to change. I thought it was convenient to publish all the packages at the same time with the same version.

No features have been added on purpose, to limit the scope of the PR.

"prettier": "^2.0.5",
"rxjs": "6.x",
"split": "^1.0.1",
"xo": "*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not change our code style.

Copy link
Author

@MarcoScabbiolo MarcoScabbiolo May 20, 2020

Choose a reason for hiding this comment

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

It was a way of updating the linter to fit the current standards and ecosystem. XO has 30k downloads vs eslint/prettier who have more than two million. A customized eslint setup with prettier and typescript-eslint is much more powerful and flexible than xo. Let's use whatever coding styles you like, but don't use outdated and unused tools for linting when you have a clear industry standard (eslint) that provides by far the best DX, features, and IDE integrations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I can't agree with you. XO is based on ESLint. It is inappropriate for you to try to compare them. We can continue to use XO's code style, and it also supports TypeScript.

Please do not remove AVA and XO, we do not want to change in this regard. Sam and I use AVA and XO for all our open-source work and we like them.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, feel free to change it. Just so you know I doubt XO has as good typescript rules as typescript-eslint. And AVA sure lacks a lot of features jest has to support this type of typescript monorepo. I wouldn't stick with them for too long if I was you.

@MarcoScabbiolo
Copy link
Author

@SamVerschueren @LitoMore any comments on this?

@SamVerschueren
Copy link
Owner

I'll try to review it shortly. Thanks for this massive PR :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants