Upgrade to v14 #2105

anandtiwary · 2023-04-04T17:20:43Z

Description

Please include a summary of the change, motivation and context.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

package.json

aaron-steinfeld · 2023-04-05T14:45:27Z

projects/observability/src/pages/explorer/explorer.component.test.ts

@@ -164,19 +164,17 @@ describe('Explorer component', () => {
        limit: 1000,
        interval: new TimeDuration(15, TimeUnit.Second)
      }),
-      expect.objectContaining({})
+      undefined


Sounds like something is broken? It used to be called with an object and now we're saying it's undefined?

Yeah, i am not sure if the tests were working as intended before. I will double check this page to see if there is any issue.

this seems like the only outstanding issue. If they were returning an object before and now returning undefined, something's been functionally changed. It may be on the test side, may be on the component side but seems like we should understand this issue before merging.

Just tested this and things seem to work as expected. Not seeing any issues at all. Imo, we can go ahead with this.

Please follow up on it - something's clearly broken, sounds like it's on the test side.

aaron-steinfeld · 2023-04-05T14:46:36Z

projects/observability/src/shared/components/cartesian/d3/cartesian-no-data-message.ts

@@ -8,7 +8,7 @@ export class CartesianNoDataMessage {

  public constructor(
    private readonly hostElement: Element,
-    private readonly series: Series<{}>[],
+    private readonly series: Series<unknown>[],


why this change?

scratch that. This is the error we get from the new ts

Argument of type 'Series<TData>[]' is not assignable to parameter of type 'Series<{}>[]'. Type 'Series<TData>' is not assignable to type 'Series<{}>'. Type 'TData' is not assignable to type '{}'.ts(2345)

adding a bound on the definition, Series<TInterval> would resolve that right?

umm.. may be, but unknown makes sense to me here. No data doesn't need to know the generic type of series data. I can define TInterval, but i think i will also have to default to unknown or always pass a type from the caller side.

In the context of the larger review, didn't look where we were. Agree, no data doesn't care (i.e. doesn't make any assumptions that it's an object) and as long as it works with unknown it's fine.

projects/observability/src/shared/components/utils/d3/zoom/d3-zoom.ts

aaron-steinfeld · 2023-04-05T14:51:27Z

...ql/request/handlers/entities/query/topology/entity-topology-graphql-query-handler.service.ts

@@ -123,8 +123,10 @@ export class EntityTopologyGraphQlQueryHandlerService
    }
  }

-  public convertResponse(response: TopologyServerResponse, request: GraphQlEntityTopologyRequest): EntityNode[] {
-    return this.convertToEntityNodes(request, response.results);
+  public convertResponse(response: unknown, request: GraphQlEntityTopologyRequest): EntityNode[] {


why did this lose typing?

We have used this pattern at a few places. We tend to avoid exporting "server" types from the handler but in this case, we must export them if we want the tests to be able to call this method.

Here is an example:

public convertResponse(response: unknown, request: GraphQlEntitiesQueryRequest): Observable<EntitiesResponse> { return this.entitiesGraphQlQueryBuilderService.buildResponse(response, request); }

Why don't we just export TopologyServerResponse from this file, if we want to use it in the test?

If we are worried about exposing an interface outside of a module, we should just not export the interface in a public-api.ts or index.ts file in the module/project.

we must export them if we want the tests to be able to call this method.

I thought it should work as long as we don't need a direct type reference?

It complains. Probably because we didn't pass all properties. I didn't want to debug and change this in the code as part of this PR. Do you mind if we address all similar occurrences in a new PR?

aaron-steinfeld · 2023-04-05T14:53:54Z

tsconfig.json

    "baseUrl": "./",
    "importHelpers": true,
    "outDir": "./dist/out-tsc",
    "strict": true,
    "sourceMap": true,
    "declaration": false,
    "module": "es2020",
-    "target": "es2015",
+    "target": "es2020",


Is changing the target required? Just curious why we're doing it now

reverted. But ng automatically had changed it to target": "ES2021",. Should we move to the new target?

Would have to go through compatibility - it doesn't really matter to us since TS handles the down-compilation, so no harm (other than missed optimizations) in targeting the older version with more compatibility

In that case, what about .browserlistrc? Just by virtue of updating Angular, we are limiting browser support to the most recent browsers at the time of Angular 14's release (Q2 2022). (See browserlist).

IMO we should update this to ES2021 or ES2020, and we should check for compatibility after updating Angular anyways, regardless of any internal SLA that might be different than Angular's (which is "the most recent browsers", presumably when Angular 14 was released last year).

I think we should decouple this one from the current PR. I will do some research and take your concern into account and put a new PR.

Agree, decouple. There's a difference in angular (and our) published SLA and best effort. Like there's no way angular actually only supports the last 2 versions of chrome (i.e. the march and april releases, but not feb), or other evergreens. That's just what they're putting in writing. We've got to be careful with older support given the nature of larger enterprise users and slower upgrade cadences.

anandtiwary · 2023-04-05T16:48:40Z

@aaron-steinfeld updated.

codecov · 2023-04-05T16:58:52Z

Codecov Report

Merging #2105 (274c141) into main (ddccf82) will decrease coverage by 42.35%.
The diff coverage is 13.79%.

@@             Coverage Diff             @@
##             main    #2105       +/-   ##
===========================================
- Coverage   83.11%   40.77%   -42.35%     
===========================================
  Files         905      905               
  Lines       19974    19769      -205     
  Branches     2861     3052      +191     
===========================================
- Hits        16602     8061     -8541     
- Misses       3221    11195     +7974     
- Partials      151      513      +362

Impacted Files	Coverage Δ
...ts/assets-library/src/icons/icon-library.module.ts	`100.00% <ø> (ø)`
projects/common/src/color/color.ts	`25.00% <ø> (ø)`
...rojects/common/src/layout/layout-change.service.ts	`45.83% <ø> (-54.17%)`	⬇️
projects/common/src/logger/logger.service.ts	`40.00% <ø> (ø)`
...ommon/src/telemetry/user-telemetry-impl.service.ts	`26.00% <ø> (-52.00%)`	⬇️
...ects/common/src/telemetry/user-telemetry.module.ts	`60.00% <ø> (ø)`
projects/common/src/time/time-duration.service.ts	`26.08% <ø> (-69.57%)`	⬇️
projects/common/src/time/time-duration.ts	`73.00% <ø> (-22.00%)`	⬇️
...ilities/browser/storage/dictionary-storage-impl.ts	`89.47% <ø> (ø)`
projects/common/src/utilities/coercers/coercer.ts	`67.74% <ø> (-29.04%)`	⬇️
... and 119 more

... and 412 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Exac

I'm not sure how thorough we want to be with this PR in the interest of time. Since we are bumping TypeScript by five minor versions, I took a look at the changelog blogposts:

The first section of the 4.4 update mentions that TS can now detect if a constant value contains a type guard, so we could improve confirmation-modal.component.ts:

    this.isContentString = isString(config.content);
    if (isString(config.content)) {
    // calling isString twice is now redundant, we can replace the second call with:
    if (this.isContentString) {

There are probably more. If our goal is just to bump the version up to catch up, then that's okay too.

Exac · 2023-04-05T16:43:27Z

...ql/request/handlers/entities/query/topology/entity-topology-graphql-query-handler.service.ts

@@ -123,8 +123,10 @@ export class EntityTopologyGraphQlQueryHandlerService
    }
  }

-  public convertResponse(response: TopologyServerResponse, request: GraphQlEntityTopologyRequest): EntityNode[] {
-    return this.convertToEntityNodes(request, response.results);
+  public convertResponse(response: unknown, request: GraphQlEntityTopologyRequest): EntityNode[] {


Why don't we just export TopologyServerResponse from this file, if we want to use it in the test?

If we are worried about exposing an interface outside of a module, we should just not export the interface in a public-api.ts or index.ts file in the module/project.

Exac · 2023-04-05T18:15:58Z

tsconfig.json

    "baseUrl": "./",
    "importHelpers": true,
    "outDir": "./dist/out-tsc",
    "strict": true,
    "sourceMap": true,
    "declaration": false,
    "module": "es2020",
-    "target": "es2015",
+    "target": "es2020",


In that case, what about .browserlistrc? Just by virtue of updating Angular, we are limiting browser support to the most recent browsers at the time of Angular 14's release (Q2 2022). (See browserlist).

IMO we should update this to ES2021 or ES2020, and we should check for compatibility after updating Angular anyways, regardless of any internal SLA that might be different than Angular's (which is "the most recent browsers", presumably when Angular 14 was released last year).

anandtiwary · 2023-04-05T18:40:30Z

I'm not sure how thorough we want to be with this PR in the interest of time. Since we are bumping TypeScript by five minor versions, I took a look at the changelog blogposts:
* https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/

* https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/

* https://devblogs.microsoft.com/typescript/announcing-typescript-4-6/

* https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/

* https://devblogs.microsoft.com/typescript/announcing-typescript-4-8/
The first section of the 4.4 update mentions that TS can now detect if a constant value contains a type guard, so we could improve confirmation-modal.component.ts:
    this.isContentString = isString(config.content);
    if (isString(config.content)) {
    // calling isString twice is now redundant, we can replace the second call with:
    if (this.isContentString) {
There are probably more. If our goal is just to bump the version up to catch up, then that's okay too.

Yeah, these are some things we should do with follow up PRs. We will also have to do similar upgrades for angular related code.

Exac · 2023-04-05T19:05:05Z

package.json

+    "eslint-config-prettier": "^8.5.0",
+    "eslint-plugin-import": "2.27.5",
+    "eslint-plugin-jsdoc": "40.0.0",
+    "eslint-plugin-no-null": "1.0.2",


Why do we install this, but then remove all the comments from tslint that enable this behavior?

// tslint:disable-next-line: no-null-keyword

I think we don't actually need to install this plugin if we removed all the comments.

Also, FWIW, I think allowing the null keyword is a great thing.

aaron-steinfeld · 2023-04-05T19:15:42Z

There are probably more. If our goal is just to bump the version up to catch up, then that's okay too.

We should bump the version in its own PR (obviously, this one's huge already), then we can go through and see what features we can take advantage of. Would be good to spend some time on weekly call going over the new features available.

Why do we install this, but then remove all the comments from tslint that enable this behavior?

I thought we had just replaced the tslint disable overrides with equivalent eslint overrides? It's still on by default right

Also, FWIW, I think allowing the null keyword is a great thing.

Separate discussion, we should try to migrate as-is then we can consider rules changes. I disagree, but certainly open to debate!

Exac · 2023-04-05T20:45:33Z

There are probably more. If our goal is just to bump the version up to catch up, then that's okay too.

We should bump the version in its own PR (obviously, this one's huge already), then we can go through and see what features we can take advantage of. Would be good to spend some time on weekly call going over the new features available.

I remember updating Angular in the past and running into issues when changing the version of typescript that the auto update tool suggested.

I think I would personally rather have the stronger typing from the newer version of TypeScript sooner, and then go back and look at improvements that we missed later. We could create some tasks to look for improvements from each minor version of TypeScript. @anandtiwary has already taken care of the breaking changes that throw compile-time errors, which is the difficult part.

Also, FWIW, I think allowing the null keyword is a great thing.

Separate discussion, we should try to migrate as-is then we can consider rules changes. I disagree, but certainly open to debate!

It would be great if we could have a linter rule smart enough to see when we are providing null to a library function outside of our source code (which is 100% of the places where we currently invoke the no-null rule. We can discuss when we have time.

github-actions · 2023-04-06T18:20:52Z

Unit Test Results

      4 files ±0   304 suites ±0 41m 40s ⏱️ + 18m 2s
1 093 tests ±0 1 093 ✔️ ±0 0 💤 ±0 0 ❌ ±0
1 101 runs ±0 1 101 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 69e7af1. ± Comparison against base commit ddccf82.

anandtiwary added 4 commits March 30, 2023 19:34

feat: upgrading to ng 13

1c4a2ef

feat: adding eslint

b6f7d82

refactor: upgrade jest

5ffd291

Merge branch 'main' into upgrade-to-v14

39b0518

This comment has been minimized.

Sign in to view

refactor: updating package lock

79f93a1

This comment has been minimized.

Sign in to view

refactor: updating test path pattern option

ff76f5c

This comment has been minimized.

Sign in to view

refactor: fix test error

b886541

This comment has been minimized.

Sign in to view

anandtiwary marked this pull request as ready for review April 5, 2023 03:52

anandtiwary requested a review from a team as a code owner April 5, 2023 03:52

arjunlalb previously approved these changes Apr 5, 2023

View reviewed changes

aaron-steinfeld reviewed Apr 5, 2023

View reviewed changes

refactor: address review comments

b3c0c05

anandtiwary dismissed arjunlalb’s stale review via b3c0c05 April 5, 2023 16:47

This comment has been minimized.

Sign in to view

Exac reviewed Apr 5, 2023

View reviewed changes

Exac self-requested a review April 5, 2023 19:00

Exac reviewed Apr 5, 2023

View reviewed changes

refactor: import ApolloModule

3d479b4

This comment has been minimized.

Sign in to view

Exac self-requested a review April 6, 2023 08:43

aaron-steinfeld approved these changes Apr 6, 2023

View reviewed changes

Merge branch 'main' into upgrade-to-v14

274c141

anandtiwary enabled auto-merge (squash) April 6, 2023 18:02

anandtiwary merged commit 69e7af1 into main Apr 6, 2023

anandtiwary deleted the upgrade-to-v14 branch April 6, 2023 18:10

Upgrade to v14 #2105

Upgrade to v14 #2105

Uh oh!

Conversation

anandtiwary commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Documentation

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anandtiwary Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anandtiwary commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

codecov bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Exac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

anandtiwary commented Apr 4, 2023 •

edited

Loading

anandtiwary Apr 5, 2023 •

edited

Loading

anandtiwary commented Apr 5, 2023 •

edited

Loading

codecov bot commented Apr 5, 2023 •

edited

Loading

Exac left a comment •

edited

Loading

aaron-steinfeld commented Apr 5, 2023 •

edited

Loading

Exac commented Apr 5, 2023 •

edited

Loading