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

build(ui): fix monaco-editor font path #11655

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Aug 23, 2023

Motivation

Modifications

  • Update to the new path to monaco-editor's font in the Webpack config

Verification

  • from argo-workflows/ui/
    • ❯ ls node_modules/monaco-editor/min/vs/base/browser/ui/codicons/codicon/codicon.ttf
      node_modules/monaco-editor/min/vs/base/browser/ui/codicons/codicon/codicon.ttf
    • also there is no codiconLabel directory anymore
  • build now passes
  • Confirmed that the editor renders with syntax highlighting (so I think it's good?):
    Screenshot 2023-08-23 at 12 18 30 AM

- the path appears to have changed in newer versions of Monaco
  - but builds seem to have still passed in efb1181
  - it then failed in aac4f89, likely because the updated `CopyWebpackPlugin` now actually errors out on a missing file
  - this `from` path is now equivalent to [Argo CD's config](https://github.com/argoproj/argo-cd/blob/24c080b5cbea6e3af547868e540caf553920dc73/ui/src/app/webpack.config.js#L89C28-L89C94)

Verification:
- from `argo-workflows/ui/`
  - ```sh
    ❯ ls node_modules/monaco-editor/min/vs/base/browser/ui/codicons/codicon/codicon.ttf
    node_modules/monaco-editor/min/vs/base/browser/ui/codicons/codicon/codicon.ttf
    ```
  - also there is no `codiconLabel` directory anymore
- build now passes

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Why was build not passing? The CI builds were successful

@agilgur5
Copy link
Member Author

agilgur5 commented Aug 23, 2023

From my opening comment:

The master branch build after merge failed

i.e. when the two PRs were merged together, there was some breakage.
I have a feeling there was breakage before then too with the fonts not working, just that it wasn't detected until we had the newer version of CopyWebpackPlugin which actually errored on build. (i.e. it was a silent error before, now it is quite loud)

@agilgur5
Copy link
Member Author

agilgur5 commented Aug 23, 2023

Also it looks like every PR based off master (except this one) is failing on CI UI 😖

@terrytangyuan terrytangyuan enabled auto-merge (squash) August 23, 2023 11:15
@terrytangyuan terrytangyuan merged commit 509b398 into argoproj:master Aug 23, 2023
25 checks passed
@agilgur5 agilgur5 deleted the fix-monaco-webpack branch August 23, 2023 16:09
@agilgur5 agilgur5 added area/ui area/build Build or GithubAction/CI issues labels Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants