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.

Sign up for GitHub

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

Jump to bottom

Ban circular dependencies #3547

Merged
merged 1 commit into from Jan 5, 2022
Merged

Ban circular dependencies #3547

merged 1 commit into from Jan 5, 2022

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Jul 30, 2021 •
edited

Signed-off-by: Sebastian Malton sebastian@malton.name

This is the last in a long chain of PRs which all need to be merged before this one will pass. This was done to make reviewing each part easier.

The PRs are as follows and must be merged in order

  1. Move cluster related types and function into seperate files #3530
  2. Move Hotbar types to seperate file #3532
  3. Split KubeObject related components into seperate folders #3533
  4. Make DetectoryRegistry a Singleton and initialize in a function #3538
  5. Move lookupApi into apiManager #3540
  6. Remove circular imports around the command pallet #3544
  7. Remove circular dependency with isAllowedResource function #3545
  8. Inject handlers into LensProxy to remove circular-deps #3546

The following PR: must also be merged before this one, but can be done out of order of the above:

  1. Cleanup integration and unit tests  #3531

blocked on above

@Nokel81 Nokel81 added the chore label Jul 30, 2021
@Nokel81 Nokel81 added this to the 5.2.0 milestone Jul 30, 2021
@Nokel81 Nokel81 requested a review from a team as a code owner July 30, 2021 17:24
@Nokel81 Nokel81 self-assigned this Jul 30, 2021
@Nokel81 Nokel81 requested review from jakolehm and aleksfront and removed request for a team July 30, 2021 17:24
@Nokel81 Nokel81 removed their assignment Aug 3, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2021

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2021

Conflicts have been resolved. A maintainer will review the pull request shortly.

Base automatically changed from lens-proxy-injections to master August 4, 2021 23:18
@Nokel81 Nokel81 requested a review from jim-docker August 5, 2021 13:08
jim-docker
jim-docker reviewed Aug 5, 2021
View reviewed changes
src/common/user-store/preferences-helpers.ts Outdated Show resolved Hide resolved
src/renderer/components/dock/dock.store.ts Outdated Show resolved Hide resolved
src/renderer/utils/createStorage.ts Outdated Show resolved Hide resolved
jim-docker
jim-docker reviewed Aug 5, 2021
View reviewed changes
src/common/user-store/preferences-helpers.ts Outdated Show resolved Hide resolved
src/common/cluster-store.ts Outdated Show resolved Hide resolved
jim-docker
jim-docker reviewed Aug 5, 2021
View reviewed changes
.eslintrc.js Outdated Show resolved Hide resolved
src/renderer/theme.store.ts Show resolved Hide resolved
@Nokel81 Nokel81 requested a review from jim-docker August 5, 2021 19:36
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2021

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Nokel81 Nokel81 modified the milestones: 5.3.3, 5.3.4, 5.4.0 Dec 9, 2021
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Nokel81
Ban circular dependencies
5d2d1fa 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

Iku-turso
Iku-turso reviewed Jan 5, 2022
View reviewed changes
src/common/ipc/ipc.ts
@@ -30,7 +30,15 @@ import { ClusterFrameInfo, clusterFrameMap } from "../cluster-frames";
import type { Disposer } from "../utils";
import type remote from "@electron/remote";

const electronRemote = ipcMain ? null : require("@electron/remote");
const electronRemote = (() => {
if (ipcRenderer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is non-env-agnostic code in common, which is bad. However, the tooling to solve something like this is not here yet -> ok for now.

Iku-turso
Iku-turso reviewed Jan 5, 2022
View reviewed changes
src/common/ipc/ipc.ts
const electronRemote = ipcMain ? null : require("@electron/remote");
const electronRemote = (() => {
if (ipcRenderer) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Add comment to explain the underlying very good reason for this wild-card try/catch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code will be going away as soon as #4625 is merged so I don't think it is really necessary.

Iku-turso
Iku-turso reviewed Jan 5, 2022
View reviewed changes
src/common/k8s-api/kube-api.ts
@@ -34,6 +34,9 @@ import type { IKubeWatchEvent } from "./kube-watch-api";
import { KubeJsonApi, KubeJsonApiData } from "./kube-json-api";
import { noop } from "../utils";
import type { RequestInit } from "node-fetch";

// BUG: https://github.com/mysticatea/abort-controller/pull/22
Copy link
Contributor

Choose a reason for hiding this comment

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

Could explain more about the relevancy of this bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there is the rule import/no-named-as-default from eslint-import which errors if (according to the typings) an default import uses the same name as a named non-default import because it might mean that you meant to import that instead.

However, in this case there is a bug in the "abort-controller" package where (even though according to the typings there is an export called "AbortController") in fact it is only default exported in the JS.

Iku-turso
Iku-turso reviewed Jan 5, 2022
View reviewed changes
src/common/k8s-api/kube-object.store.ts
@@ -30,6 +30,9 @@ import { ensureObjectSelfLink, IKubeApiQueryParams, KubeApi } from "./kube-api";
import { parseKubeApi } from "./kube-api-parse";
import type { KubeJsonApiData } from "./kube-json-api";
import type { RequestInit } from "node-fetch";

// BUG: https://github.com/mysticatea/abort-controller/pull/22
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before.

Iku-turso
Iku-turso reviewed Jan 5, 2022
View reviewed changes
src/renderer/components/kube-object-details/kube-object-details.tsx
@@ -62,7 +61,7 @@ export class KubeObjectDetails extends React.Component {
.getStore(this.path)
?.getByPath(this.path);
} catch (error) {
logger.error(`[KUBE-OBJECT-DETAILS]: failed to get store or object: ${error}`, { path: this.path });
console.error(`[KUBE-OBJECT-DETAILS]: failed to get store or object: ${error}`, { path: this.path });
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Should not use explicit console.error instead of abstracted logger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, I don't exactly remember why I did this since in #4317 I revert this back.... will revert.

Iku-turso
Iku-turso approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@Iku-turso Iku-turso left a comment

Choose a reason for hiding this comment

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

Added mostly superficial comments. Mergeable without action.

jim-docker
jim-docker approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

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

for me, make lint now takes 67s (was 17s) on mac, 54s (was 12s) on linux. Much better than 10m

@Nokel81 Nokel81 merged commit 4f75acf into master Jan 5, 2022
@Nokel81 Nokel81 deleted the ban-cir-deps branch January 5, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Iku-turso Iku-turso Iku-turso approved these changes

@jim-docker jim-docker jim-docker approved these changes

@jakolehm jakolehm Awaiting requested review from jakolehm

@aleksfront aleksfront Awaiting requested review from aleksfront

Assignees

@jim-docker jim-docker

Labels
chore p1
Projects
None yet
Milestone
5.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Nokel81 @Iku-turso @jim-docker @jakolehm