Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(build): add more TypeScript "strict" checks #2704

Merged
merged 1 commit into from Apr 17, 2019

Conversation

Projects
None yet
2 participants
@bajtos
Copy link
Member

commented Apr 8, 2019

Enable all "strict" checks with the exception of strictPropertyInitialization which would require significant changes.

The following additional checks are added:

  • noImplicitThis
  • alwaysStrict
  • strictFunctionTypes

In the future, any checks added to TypeScript "strict" mode will be automatically enabled for LoopBack projects too.

See also TypeScript Compiler Options

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • 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

馃憠 Check out how to submit a PR 馃憟

@bajtos bajtos self-assigned this Apr 8, 2019

@bajtos bajtos requested review from raymondfeng and strongloop/loopback-maintainers Apr 8, 2019

@bajtos bajtos force-pushed the feat/tsc-strict-checks branch from 82cdcd6 to 234ec10 Apr 8, 2019

@bajtos bajtos referenced this pull request Apr 9, 2019

Merged

feat(context): always pass the session to ResolverFunction #2711

2 of 2 tasks complete
@raymondfeng

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Now #2711 is landed, please rebase against latest master.

@bajtos bajtos force-pushed the feat/tsc-strict-checks branch from 234ec10 to af016a4 Apr 11, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Now #2711 is landed, please rebase against latest master.

Done.

I have also changed the way how I address the problem with BindingFilter<T> vs. BindingFilter<unknown>, see 90117fa

@raymondfeng LGTY now?

@bajtos bajtos force-pushed the feat/tsc-strict-checks branch 2 times, most recently from bfffe47 to a18b41b Apr 11, 2019

@raymondfeng raymondfeng referenced this pull request Apr 11, 2019

Merged

feat(context): fix generic typing for BindingFilter #2728

1 of 7 tasks complete
* We learned about this problem after enabling TypeScript's `strictFunctionTypes`
* check, but decided to preserve `ValueType` argument for backwards compatibility.
*
* TODO(semver-major): remove ValueType template argument.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 11, 2019

Member

See #2728. The ValueType is used internally with a default. Applications or extensions may define their own filter functions and mostly won't cast it to BindingFilter<...>. I don't think we have to release a new major.

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Is there a possibility that this change will break existing applications?

@@ -161,10 +161,10 @@ export class ContextView<T = unknown> extends EventEmitter
*/
export function createViewGetter<T = unknown>(
ctx: Context,
bindingFilter: BindingFilter<T>,
bindingFilter: BindingFilter,

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Author Member

Before this change, it was possible to infer T in createViewGetter from bindingFilter type.

const filter: BindingFilter<MyValue> = b => true;
const getter = createViewGetter(ctx, filter);
// ^^^ getter is of type Getter<MyValue[]>

With this change in place, users have to explicitly specify the getter type, otherwise they end up with unknown.

const getter = createViewGetter(ctx, filter);
// ^^^ getter is of type Getter<unknown[]>

const getter = createViewGetter<MyValue>(ctx, filter);
// ^^^ getter is of type Getter<MyValue[]>
session?: ResolutionSession,
): Getter<T[]> {
const view = new ContextView(ctx, bindingFilter);
const view = new ContextView<T>(ctx, bindingFilter);

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 12, 2019

Author Member

Similarly here. Before this change, it was possible to infer T of ContextView from bindingFilter type. After this change, new ContextView(...) always treats T as unknown.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Is there a possibility that this change will break existing applications?

Good question, see my two comments above.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Also any project using @loopback/build and our shared tsconfig may start failing to compile if they are not violating any of the newly enabled checks.

I don't consider this as a change that would require semver-major, because this situation happens regularly with new semver-minor releases of TypeScript. TypeScript releases a new version, e.g. 3.4, consumers upgrade to this new version (because @loopback/build uses ^3.3.0 as the dependency spec) automatically, and because the new version of compiler is rejecting code that was accepted by the old version, build suddenly breaks. I am arguing that build broken by a new "strict" rule enabled by @loopback/build is conceptually the same as when the build is broken by a new version of tsc.

@bajtos bajtos referenced this pull request Apr 12, 2019

Merged

Improve type safety of function calls #2733

3 of 3 tasks complete
@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

I have extracted the first two commits to get them landed faster: #2733

@bajtos bajtos force-pushed the feat/tsc-strict-checks branch 2 times, most recently from ed788bf to e37e5f4 Apr 15, 2019

feat(build): add more TypeScript "strict" checks
Enable all "strict" checks with the exception of
`strictPropertyInitialization` which would require significant changes.

The following additional checks are added:

- noImplicitThis
- alwaysStrict
- strictFunctionTypes

In the future, any checks added to TypeScript "strict" mode will be
automatically enabled for LoopBack projects too.

@bajtos bajtos force-pushed the feat/tsc-strict-checks branch from e37e5f4 to 70af431 Apr 16, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

@raymondfeng I have rebased the pull request on top of the latest master, it's ready for final review & landing. LGTY?

@bajtos bajtos requested a review from raymondfeng Apr 16, 2019

@raymondfeng raymondfeng merged commit 866aa2f into master Apr 17, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 91.244%
Details

@raymondfeng raymondfeng deleted the feat/tsc-strict-checks branch Apr 17, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@bajtos I landed for you to reduce the backlog of PRs.

raymondfeng added a commit that referenced this pull request Apr 17, 2019

raymondfeng added a commit that referenced this pull request Apr 19, 2019

raymondfeng added a commit that referenced this pull request Apr 22, 2019

raymondfeng added a commit that referenced this pull request Apr 23, 2019

raymondfeng added a commit that referenced this pull request Apr 29, 2019

raymondfeng added a commit that referenced this pull request May 3, 2019

raymondfeng added a commit that referenced this pull request May 3, 2019

raymondfeng added a commit that referenced this pull request May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.