search

argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.94k stars 5.46k forks source link

ArgoUI: ArgoCD fails to build due to refactoring changes to correct typing errors #4913

Closed keithchong closed 3 years ago

keithchong commented 3 years ago

If you are trying to resolve an environment-specific issue or have a one-off question about the edge case that does not require a feature then please consider asking a question in argocd slack channel.

Checklist:

Describe the bug

ArgoCD fails to build if you pull in the latest from argo-ui. One of the errors is due to a missing ? on the input property in data-loader.tsx

To Reproduce

Get the latest argo-ui and argo-cd in your dev environment and build argo-cd.

You will see several errors including:

ERROR in  <path>/argoproj/argo-cd/ui/src/app/app.tsx
./src/app/app.tsx
[tsl] ERROR in  <path>/argoproj/argo-cd/ui/src/app/app.tsx(190,62)
      TS2741: Property 'input' is missing in type '{ children: (version: VersionMessage) => Element; load: () => Promise<VersionMessage>; }' but required in type 'Readonly<LoaderProps<undefined, VersionMessage>>'.
.
.
.
ERROR in  <path>/argoproj/argo-cd/ui/src/app/applications/components/application-sync-options.tsx
./src/app/applications/components/application-sync-options.tsx
[tsl] ERROR in  <path>/argoproj/argo-cd/ui/src/app/applications/components/application-sync-options.tsx(38,14)
      TS2322: Type 'import(" <path>/argoproj/argo-cd/ui/node_modules/@types/react/index").CSSProperties' is not assignable to type 'React.CSSProperties'.

For the first error, the input property should still be optional (? mark is missing). See: https://github.com/argoproj/argo-ui/pull/72/files# src/components/data-loader.tsx

Expected behavior argo-cd "should" build cleanly if I want to pull in the master (or release) branch of argo-ui ( https://github.com/argoproj/argo-cd/issues/4578 ) and if there are no corresponding dependency changes on the argo-cd side.

Version Grab the latest from argo-ui and argo-cd

dcherman commented 3 years ago

@keithchong Mind taking a look? https://github.com/argoproj/argo-ui/pull/76/files

I think I covered all use cases for input/load combinations there including:

keithchong commented 3 years ago

Hi @dcherman , I applied your changes (argocd built fine) and tested it out but I'm getting an invalid hook call. The argocd application page does not render correctly and shows a 'Something went wrong!' message. Here is the stack trace.

react.development.js:1465 Uncaught Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:

  1. You might have mismatching versions of React and the renderer (such as React DOM)
  2. You might be breaking the Rules of Hooks
  3. You might have more than one copy of React in the same app See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem. at resolveDispatcher (react.development.js:1465) at Object.useState (react.development.js:1496) at Autocomplete (autocomplete.tsx:39) at renderWithHooks (react-dom.development.js:12938) at mountIndeterminateComponent (react-dom.development.js:15020) at beginWork (react-dom.development.js:15625) at performUnitOfWork (react-dom.development.js:19312) at workLoop (react-dom.development.js:19352) at HTMLUnknownElement.callCallback (react-dom.development.js:149) at Object.invokeGuardedCallbackDev (react-dom.development.js:199)
dcherman commented 3 years ago

I think I know why; React is incorrectly a dependency in argo-ui versus a peerDependency that it should be since React expects there to be a single version on a given page (https://github.com/facebook/react/issues/13991). We should likely migrate to using a peerDependency for react and react-dom and allow the decision for exactly what version to load be made by the consumers (in this case, argo and argo-cd).

keithchong commented 3 years ago

Confirmed it is a React version mismatch. I updated the React version on argo-cd side to match what argo-ui was using, and the page rendered correctly.

dcherman commented 3 years ago

Down the dependency rabbit hole I go :). There are larger problems with dependencies than just React, I'm currently working my way through them. If we need to get this fixed in the short term (read: less than a day, I hope to get a proposed fix quickly), then I'd suggest we revert the type fix PR.

As an example, argo-cd uses but does not declare a dependency on rxjs, and that started causing issues when I fixed the dependency on React. I'm hoping to work my way through all of this relatively quickly, but it might take me a day or two since I need to make sure I also don't break Argo Workflows at the same time.

dcherman commented 3 years ago

PR updated to move React to a peerDependency and confirmed that both Argo and Argo-CD build when my branch is used as the target for that. Most of the typing errors I was experiencing was due to testing it by using yarn add file:../argo-ui which resulted in type errors when compiling - not really sure why.

There are still most definitely problems with how the dependencies are set up that I'll try to work my way through, but I don't think those fixes are required to solve this.

Note that react and react-dom should be updated to at least 16.9.3 to match the peerDependency in argo-ui after my PR in argo-ui is merged. The same is also true in Argo Workflows.

keithchong commented 3 years ago

All, and @alexmt, the argo-cd PR that will pull in the latest changes from argo-ui, including the above refactor-type and peerDependency changes, is https://github.com/argoproj/argo-cd/pull/4909. All checks have passed. The PR also includes the updates to the react and react-dom versions (See package.json and yarn.lock files). I've tested the UI manually, created a project and sync-ed successfully. The application list page did render properly.

However, during my investigation, I came across an issue. The application list page populated but the filter on the left (or top) failed to render with the following error. Note, I cannot reproduce this problem with the latest changes in the PR. Please give it a try to see if you can reproduce.

main.204346fd671ab36d1385.js:131814 Uncaught (in promise) TypeError: callback is not a function
    at flushFirstCallback (main.204346fd671ab36d1385.js:131814)
    at flushImmediateWork (main.204346fd671ab36d1385.js:131876)
    at unstable_runWithPriority (main.204346fd671ab36d1385.js:131968)
    at runWithPriority$1 (main.204346fd671ab36d1385.js:79269)
    at flushSyncCallbackQueueImpl (main.204346fd671ab36d1385.js:79314)
    at flushSyncCallbackQueue (main.204346fd671ab36d1385.js:79302)
    at scheduleUpdateOnFiber (main.204346fd671ab36d1385.js:89429)
    at Object.enqueueSetState (main.204346fd671ab36d1385.js:80869)
    at DataLoader../node_modules/react/cjs/react.development.js.Component.setState (main.204346fd671ab36d1385.js:101757)
    at DataLoader../node_modules/argo-ui/src/components/data-loader.tsx.DataLoader.handleError (main.204346fd671ab36d1385.js:2336)
alexmt commented 3 years ago

Running UI with changes from https://github.com/argoproj/argo-cd/pull/4909 locally. I cannot reproduce the issue as well. Even if issue happens again we have time to fix in 1.9. I think we should merge https://github.com/argoproj/argo-cd/pull/4909.

keithchong commented 3 years ago

Thanks @alexmt, closing this issue since the original problem (build failure and follow-on issue that the page is not rendering) has been resolved. We should open a new issue for any new problems we find.