Fix lint issues when bumping eslint-plugin-import #1652

Kavinjsir · 2023-08-09T23:24:45Z

Which problem is this PR solving?

Resolves [deps] Help with upgrading eslint-plugin-import #1613
Part of [chore]: Fix eslint warnings #1608

Description of the changes

Update code lines to resolve lint check warnings

How was this change tested?

yarn run esilnt

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality

yurishkuro · 2023-08-10T00:06:12Z

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.test.js

@@ -1,4 +1,4 @@
-// Copyright (c) 2017 Uber Technologies, Inc.
+// Copyright (c) 2023 Uber Technologies, Inc.


why is this changed?

I am wondering if we need to keep the year up to date if we have the code lines in the file get updated?
Or do we keep them as origin?

Uber's was the original copyright, new contributions should use "The Jaeger Authors", which you can add as a line above with the current year if you want.

We tend to keep the original year a specific (c) by-line was introduced. I checked with legal once, there wasn't a lot of value in updating the year.

Sounds good! I'm going to roll back the corresponding change as it is not necessary. Thx for letting me know:)

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.tsx

Kavinjsir · 2023-08-10T01:09:35Z

I think I might miss the point of ci error before. Just realized that unit-test in ci failed by running yarn lint:
https://github.com/jaegertracing/jaeger-ui/actions/runs/5815079038/job/15766674298

It seems we can reproduce the issue locally by removing the --cache flag when running eslint. Let me try resolving them.

yurishkuro · 2023-08-10T02:39:36Z

packages/jaeger-ui/src/demo/trace-generators.js

@@ -14,7 +14,7 @@

 import Chance from 'chance';

-import { getSpanId } from '../selectors/span';
+import getSpanId from '../selectors/span';


this is an exceedingly silly selector, is it actually used, or can we delete it?

yurishkuro · 2023-08-10T02:40:54Z

packages/jaeger-ui/src/selectors/trace.test.js

@@ -13,7 +13,7 @@
 // limitations under the License.

 import { followsFromRef } from './trace.fixture';
-import { getSpanId } from './span';
+import getSpanId from './span';


if this is the only place, let's just replace it with span.ID and delete the selector

Sounds good!

yurishkuro · 2023-08-10T02:43:12Z

your description links to issues, but does not put the word Fixes or Resolves in front of them. Please update, this way when the PR is merged the issues will be closed (unless it does not resolve them completely, in which case we typically use the format Part of #xxxx).

yurishkuro · 2023-08-10T02:45:16Z

package.json

@@ -14,7 +14,7 @@
    "eslint": "8.46.0",
    "eslint-config-airbnb": "19.0.4",
    "eslint-config-prettier": "9.0.0",
-    "eslint-plugin-import": "2.27.5",
+    "eslint-plugin-import": "2.28.0",


once the CI is green, I would prefer to remove the version upgrade and only leave the code changes, then let the dependabot to redo its PR for the actual upgrade.

- Update code lines based on lint check, avoid error and warning. - Remove `getSpanId` which is unnecessary. Signed-off-by: Tony Jin <kavinjsir@gmail.com>

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro · 2023-08-10T15:29:18Z

@Kavinjsir I removed the upgrade to keep the scope to code changes only, and it fails the CI, can you please check why?

Kavinjsir · 2023-08-10T15:53:43Z

@Kavinjsir I removed the upgrade to keep the scope to code changes only, and it fails the CI, can you please check why?

@yurishkuro thx for helping with the updates for dep version and pr description!

Let me take a look for the CI. Seems like some deprecated usage from old react, not sure why occurs though..

Make mock `Blob` a class Signed-off-by: Tony Jin <kavinjsir@gmail.com>

Kavinjsir · 2023-08-10T17:02:56Z

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.test.js

+          constructor(text, options) {
+            this.text = text;
+            this.options = options;
+          }


@yurishkuro From my local, it seems like what really brought the unit-test failure is:

(Not sure why it was missed in the CI logs)

For this error above, I think I made a mistake to convert Blob to an arrow function that is not able to call by new.

So I change such global.Blob to an "explicit" class. Assuming this can:

Still resolve the origin lint warning

Attempt to make it clear so satisfy calling by keyword new.

How do you think of this?

codecov · 2023-08-10T17:06:00Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (f402705) 96.01% compared to head (910694c) 96.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1652      +/-   ##
==========================================
- Coverage   96.01%   96.01%   -0.01%     
==========================================
  Files         242      241       -1     
  Lines        7559     7558       -1     
  Branches     1984     1984              
==========================================
- Hits         7258     7257       -1     
  Misses        301      301

Files Changed	Coverage Δ
...src/components/TracePage/TraceStatistics/index.tsx	`86.86% <ø> (ø)`
...components/SearchTracePage/SearchResults/index.tsx	`86.15% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2023-08-10T17:24:20Z

Thanks, @Kavinjsir, the dependabot PR #1609 was successfully merged after these changes.

🎉

Kavinjsir requested review from everett980 and rubenvp8510 as code owners August 9, 2023 23:24

Kavinjsir mentioned this pull request Aug 9, 2023

[deps] Help with upgrading eslint-plugin-import #1613

Closed

yurishkuro reviewed Aug 10, 2023

View reviewed changes

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.tsx Show resolved Hide resolved

Kavinjsir changed the title ~~Bump eslint-plugin-import and fix lint warning~~ WIP: Bump eslint-plugin-import and fix lint warning Aug 10, 2023

Kavinjsir force-pushed the refactor/apply-latest-eslint-plugin-import branch from 7eb3e68 to 2358668 Compare August 10, 2023 02:31

Kavinjsir changed the title ~~WIP: Bump eslint-plugin-import and fix lint warning~~ Bump eslint-plugin-import and fix lint warning Aug 10, 2023

yurishkuro reviewed Aug 10, 2023

View reviewed changes

Kavinjsir changed the title ~~Bump eslint-plugin-import and fix lint warning~~ Fix lint issues when bumping eslint-plugin-import Aug 10, 2023

Fix lint issue when bumping eslint-plugin-import.

642c8e0

- Update code lines based on lint check, avoid error and warning. - Remove `getSpanId` which is unnecessary. Signed-off-by: Tony Jin <kavinjsir@gmail.com>

Kavinjsir force-pushed the refactor/apply-latest-eslint-plugin-import branch from 2358668 to 642c8e0 Compare August 10, 2023 13:44

Undo upgrade

a38e481

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro previously approved these changes Aug 10, 2023

View reviewed changes

yurishkuro enabled auto-merge (squash) August 10, 2023 15:20

yurishkuro disabled auto-merge August 10, 2023 15:28

Fix CI failure

910694c

Make mock `Blob` a class Signed-off-by: Tony Jin <kavinjsir@gmail.com>

Kavinjsir dismissed yurishkuro’s stale review via 910694c August 10, 2023 16:50

yurishkuro approved these changes Aug 10, 2023

View reviewed changes

yurishkuro enabled auto-merge (squash) August 10, 2023 16:59

Kavinjsir commented Aug 10, 2023

View reviewed changes

yurishkuro merged commit 153ad9f into jaegertracing:main Aug 10, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lint issues when bumping eslint-plugin-import #1652

Fix lint issues when bumping eslint-plugin-import #1652

Kavinjsir commented Aug 9, 2023 •

edited by yurishkuro

yurishkuro Aug 10, 2023

Kavinjsir Aug 10, 2023

yurishkuro Aug 10, 2023

Kavinjsir Aug 10, 2023

Kavinjsir commented Aug 10, 2023

yurishkuro Aug 10, 2023

yurishkuro Aug 10, 2023

Kavinjsir Aug 10, 2023

yurishkuro commented Aug 10, 2023 •

edited

yurishkuro Aug 10, 2023

yurishkuro commented Aug 10, 2023

Kavinjsir commented Aug 10, 2023 •

edited

Kavinjsir Aug 10, 2023

codecov bot commented Aug 10, 2023

yurishkuro commented Aug 10, 2023 •

edited

		@@ -1,4 +1,4 @@
		// Copyright (c) 2017 Uber Technologies, Inc.
		// Copyright (c) 2023 Uber Technologies, Inc.

Fix lint issues when bumping eslint-plugin-import #1652

Fix lint issues when bumping eslint-plugin-import #1652

Conversation

Kavinjsir commented Aug 9, 2023 • edited by yurishkuro

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kavinjsir commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Aug 10, 2023 • edited

Choose a reason for hiding this comment

yurishkuro commented Aug 10, 2023

Kavinjsir commented Aug 10, 2023 • edited

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2023

Codecov Report

yurishkuro commented Aug 10, 2023 • edited

Kavinjsir commented Aug 9, 2023 •

edited by yurishkuro

yurishkuro commented Aug 10, 2023 •

edited

Kavinjsir commented Aug 10, 2023 •

edited

yurishkuro commented Aug 10, 2023 •

edited