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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(build): upgrade to TypeScript 3.2.1 #2098

Merged
merged 2 commits into from Nov 30, 2018
Merged

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Nov 29, 2018

See https://blogs.msdn.microsoft.com/typescript/2018/11/29/announcing-typescript-3-2/

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

Object.assign(w, clause);
}
return w;
return clause;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, are you sure this change is preserving current behavior?

A test case to consider:

cast({and: [{x: 1}, {y: 2}], z: 3})

Before your change, cast returns

{and: [{x: 1}, {y: 2}]}

After your change, cast returns

{and: [{x: 1}, {y: 2}], z: 3}

I am typing these examples from my head based on how I understand the code. I may be wrong.

@raymondfeng please take a second look at this change.

If the new implementation of cast is just an identity function (returning the input without any change), then I think we should mark cast as deprecated (at minimum in TSDocs; ideally also at runtime using depd) and remove it in the next major version.

This does not necessarily has to happen in this pull request, but it would be great to make those changes soon, otherwise we are increasing our technical debt.

Thoughts?

@@ -480,13 +472,14 @@ export class FilterBuilder<MT extends object = AnyObject> {
if (!this.filter.fields) {
this.filter.fields = {};
}
const _fields = this.filter.fields;
Copy link
Member

Choose a reason for hiding this comment

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

Why underscore? Can we use fields or perhaps allFields/filterFields to better distinguish from field name used later?

@bajtos
Copy link
Member

bajtos commented Nov 30, 2018

Nice!

I am proposing to make the following additional changes while upgrading to TS 3.2:

  1. Enable strictBindCallApply (docs)

  2. Leverage Configuration inheritance via node_modules packages (docs) to simplify tsconfig.build.json in our packages & examples, see e.g. packages/core and examples/todo.

    I think we should use the following form everywhere:

    "extends": "@loopback/build/config/tsconfig.common.json",
    

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Considering that our build is failing without some of these changes, I am ok to land this pull request as-is in order to fix our failing builds.

I wish the change-set could have been smaller though, I am not confident that this patch is fully backwards compatible 😟

@raymondfeng raymondfeng merged commit af25dc3 into master Nov 30, 2018
@raymondfeng raymondfeng deleted the upgrade-to-ts-3.2 branch November 30, 2018 16:58
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.

None yet

2 participants