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.

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

Extract tslint config into a standalone package #2159

Merged
merged 3 commits into from Dec 14, 2018

Conversation

Projects
None yet
3 participants
@bajtos
Copy link
Member

bajtos commented Dec 14, 2018

See #2156

By keeping the tslint config in an independently versioned package @loopback/tslint-config, we can properly communicate breaking changes without having to worry about long-term support for multiple versions of @loobpack/build.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated
feat: move tslint config into a standalone package
Introduce a new package "@loopback/tslint-config" and move both
configuration files to this new package.

@bajtos bajtos added the refactor label Dec 14, 2018

@bajtos bajtos self-assigned this Dec 14, 2018

@bajtos bajtos requested review from raymondfeng , b-admike and strongloop/loopback-maintainers Dec 14, 2018

@bajtos bajtos force-pushed the extract-tslint-config branch from 41c6a79 to 851ccac Dec 14, 2018

@b-admike
Copy link
Member

b-admike left a comment

💯

@@ -13,6 +13,7 @@
"copyright.owner": "IBM Corp.",
"license": "MIT",
"dependencies": {
"@loopback/tslint-config": "^1.0.0-1",

This comment has been minimized.

@b-admike

b-admike Dec 14, 2018

Member

Hmm why is this dependency^1.0.0-1 as opposed to ^1.0.0?

This comment has been minimized.

@bajtos

bajtos Dec 14, 2018

Member

tslint-config was not released yet. I specified the version number as a pre-release version, hoping that lerna will automatically increment it to 1.0.0 when we run the release process. Should I use a different version number?

This comment has been minimized.

@b-admike

b-admike Dec 14, 2018

Member

No it's fine to leave it as is, just wanted to know the reasoning behind it.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 14, 2018

AppVeyor's build is failing on experimental warning printed by Node.js for http2 module. AppVeyor is using Node.js 8.12.0 in which http2 is still experimental. Starting from 8.13.0, http2 is marked as stable.

@raymondfeng any idea which of our code is calling http2 APIs? I don't remember any pull request that would use http2 directly.

@b-admike b-admike referenced this pull request Dec 14, 2018

Merged

docs(repository): add hasOne relation docs #2161

3 of 7 tasks complete
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Dec 14, 2018

Do we want to extend this module to include tsconfig too? Please note that TypeScript 3.2.x allows references to module names for tsconfig now. If so, the module name can be @loopback/ts-config.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 14, 2018

Do we want to extend this module to include tsconfig too? Please note that TypeScript 3.2.x allows references to module names for tsconfig now. If so, the module name can be @loopback/ts-config.

No, I want to decouple tslint config as much as possible. We can take a look at a shared tsconfig later, I don't have a strong opinion whether it would be useful to have a standalone package for tsconfig or not.

bajtos added some commits Dec 14, 2018

chore: use tslint config from the new package
Reconfigure all relevant packages and the monorepo root to load
the shared tslint config from `@loopback/tslint-config`.

@bajtos bajtos force-pushed the extract-tslint-config branch from 851ccac to dcde6c7 Dec 14, 2018

@bajtos bajtos merged commit 5b9c329 into master Dec 14, 2018

4 of 5 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 90.305%
Details
security/snyk - package.json (dhmlau) No new issues
Details

@bajtos bajtos deleted the extract-tslint-config branch Dec 14, 2018

"copyright.owner": "IBM Corp.",
"license": "MIT",
"peerDependencies": {
"tslint": ">=5.11.0"

This comment has been minimized.

@bajtos

bajtos Dec 14, 2018

Member

Hmm. The idea behind this peer dependency is to specify the minimum version of tslint that can understand our rules. Unfortunately, a typical LB4 project does not install tslint directly but gets it from @loopback/build instead.

As a result, npm install can print the following warning:

npm WARN @loopback/tslint-config@1.0.0-1 requires a peer of tslint@>=5.11.0 but none is installed. You must install peer dependencies yourself.

I am not sure what to do about it. We can remove this peer dependency, but that way we loose control over tslint version used with our config. Or we can modify our project templates and example projects to install tslint directly, kind of duplicating @loopback/build's dependency on tslint.

I think in the long term, it may be best to get rid of lb-tslint and use https://github.com/Microsoft/tslint-microsoft-contrib to invoke tslint as part of tsc build.

@strongloop/loopback-maintainers thoughts?

This comment has been minimized.

@bajtos

bajtos Dec 14, 2018

Member

What's worse, some 3rd-party tslint rules like tslint-consistent-codestyle (see #2156) are loading tslint directly and fail if it's not installed as a peer dependency (e.g. because it's sourced from @loopback/build). I'll have to deal with this problem in #2156.

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