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

fix: Upgrade argo-ui to fix intermediate parameters window. Fixes #11448 #11510

Closed
wants to merge 2 commits into from

Conversation

terrytangyuan
Copy link
Member

@terrytangyuan terrytangyuan commented Aug 3, 2023

Incorporates fix in argoproj/argo-ui#418. Closes #11448

@terrytangyuan terrytangyuan enabled auto-merge (squash) August 3, 2023 00:48
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

Build failures in the tests

@terrytangyuan
Copy link
Member Author

This doesn't seem like a simple bump. @atomicman57 Would you like to help take a look at the UI build failure? https://github.com/argoproj/argo-workflows/actions/runs/5745283948/job/15573063598?pr=11510

Build seems successfully in argo-ui repo but there are errors in node_modules/argo-ui/src/components when building in argo-workflows CI.

@atomicman57
Copy link

Taking a look @terrytangyuan

@isubasinghe
Copy link
Member

isubasinghe commented Aug 13, 2023

@terrytangyuan failing to replicate on my end, perhaps this is just a random incident. Could you push an empty commit and see what the CI does?

I can build locally but cannot get act to work. This might just be because act is slightly different. It fails even on the master branch, I do not suspect this to be the cause. https://github.com/nektos/act

@terrytangyuan
Copy link
Member Author

Still seeing the errors after rebasing or triggering CI builds

@agilgur5
Copy link
Contributor

agilgur5 commented Aug 15, 2023

I was able to reproduce this locally -- did a fresh rebuild of my devcontainer with all Node 20 changes etc (directly on this branch, to be exact).

@terrytangyuan failing to replicate on my end

@isubasinghe I wonder if on your end that might be because the Nix Node 20 change hasn't been merged yet? So if you're using the Nix env based off this PR, it might be inconsistent / out-of-date

@agilgur5
Copy link
Contributor

For reference, pulling here from Slack that these are all ESM related.
And also apparently argo-ui does not export compiled code, it exports src TSX, which can result in discrepancies when different TS settings (such as esModuleInterop, which is causing errors as below) or versions are used (among potentially other issues).

There are a few different kinds of errors here:

  1. ERROR in ./node_modules/react-toastify/dist/react-toastify.esm.mjs 303:11-23
    Can't import the named export 'cloneElement' from non EcmaScript module (only default export is available)
     @ ./node_modules/argo-ui/src/components/notifications/notifications.tsx
     @ ./node_modules/argo-ui/src/components/index.ts
     @ ./node_modules/argo-ui/src/index.ts
     @ ./src/app/app.tsx
     @ ./src/app/index.tsx

    All errors of this format seem to be due to mixed React imports. This might be fixable by force resolving react-toastify to its CJS dist file (if it has one). Otherwise this may require consistent deps (there were a few installation warnings on peerDeps) or force resolving react itself.

  2. ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/node_modules/argo-ui/src/components/nav-bar/nav-bar.tsx
    ./node_modules/argo-ui/src/components/nav-bar/nav-bar.tsx
    [tsl] ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/node_modules/argo-ui/src/components/nav-bar/nav-bar.tsx(1,9)
          TS2305: Module '"classnames"' has no exported member 'default'.
    ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/node_modules/argo-ui/v2/utils/utils.tsx
    ./node_modules/argo-ui/v2/utils/utils.tsx
    [tsl] ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/node_modules/argo-ui/v2/utils/utils.tsx(1,8)
        TS1259: Module '"/home/vscode/go/src/github.com/argoproj/argo-workflows/ui/node_modules/moment/ts3.1-typings/moment"' can only be default-imported using the 'esModuleInterop' flag

    All errors of both of these formats are fixed by setting "esModuleInterop": true in tsconfig.json, as the error states.
    As I mentioned above, this is because argo-ui exports source TS instead of compiled JS and as such the tsconfig.json must match

  3. ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/src/app/event-sources/components/event-source-list/event-source-list.tsx
    ./src/app/event-sources/components/event-source-list/event-source-list.tsx
    [tsl] ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/src/app/event-sources/components/event-source-list/event-source-list.tsx(135,51)
          TS2349: This expression is not callable.
      Type '{ default: (...args: ArgumentArray) => string; }' has no call signatures.

    All errors of this format need to be changed from a namespace import (e.g. import * as classNames from ...) to a default import (e.g. import classNames from ...)

  4. ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx
    ./src/app/workflows/components/workflow-timeline/workflow-timeline.tsx
    [tsl] ERROR in /home/vscode/go/src/github.com/argoproj/argo-workflows/ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx(138,37)
          TS2349: This expression is not callable.
      Type 'typeof import("/home/vscode/go/src/github.com/argoproj/argo-workflows/ui/node_modules/moment/ts3.1-typings/moment.d.ts")' has no call signatures.

    Same as above, errors of this format can be fixed by using a default import: import moment from 'moment';

@terrytangyuan
Copy link
Member Author

terrytangyuan commented Aug 15, 2023

@agilgur5 Will create a new PR for this so I am closing this one. Thanks @agilgur5!

auto-merge was automatically disabled August 15, 2023 17:51

Pull request was closed

@agilgur5
Copy link
Contributor

  1. All errors of this format seem to be due to mixed React imports. This might be fixable by force resolving react-toastify to its CJS dist file (if it has one). Otherwise this may require consistent deps (there were a few installation warnings on peerDeps) or force resolving react itself.

For reference, these all ended up being due to usage of old Webpack v4 here. This required force resolving react-toastify to 9.0.3 per fkhadra/react-toastify#775 (comment) (the issue talks about CRA, but this seems to impact (be caused by?) upstream Webpack). Notably, Argo CD uses Webpack v5.
(In general, the UI build process could use some love and updating; I'd probably suggest moving to esbuild similar to Argo CD as the build is currently very slow)

Also fixed the deprecation warning from the old foundation-sites dep by upgrading it.

Will have a replacement PR out shortly

@agilgur5
Copy link
Contributor

Fixed all of the above in #11585

@isubasinghe
Copy link
Member

@isubasinghe I wonder if on your end that might be because the #11506 hasn't been merged yet? So if you're using the Nix env based off this PR, it might be inconsistent / out-of-date

Oh yes that could be the reason.

Nice work on getting the PR for this out though!

@terrytangyuan terrytangyuan deleted the upgrade-argo-ui branch September 26, 2023 13:25
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies area/ui javascript Pull requests that update Javascript dependencies labels Oct 18, 2023
@agilgur5 agilgur5 added the solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui javascript Pull requests that update Javascript dependencies solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: Intermediate parameters window is not scrollable
5 participants