Skip to content

Conversation

@mengw15
Copy link
Contributor

@mengw15 mengw15 commented Oct 8, 2024

This PR upgrades the Monaco Language Client from the previous version to 8.8.3. It includes upgrades to related libraries to ensure compatibility with the new version. The implementation has been adjusted to align with the new API.

@mengw15 mengw15 marked this pull request as draft October 8, 2024 23:29
@mengw15 mengw15 self-assigned this Oct 9, 2024
@mengw15 mengw15 changed the title monaco upgrade Upgrade Monaco Language Client to 8.8.3 Oct 10, 2024
@mengw15 mengw15 marked this pull request as ready for review October 10, 2024 18:19
@mengw15 mengw15 requested a review from aglinxinyuan October 10, 2024 18:19
@mengw15 mengw15 added frontend Changes related to the frontend GUI dependencies Pull requests that update a dependency file labels Oct 10, 2024
Copy link
Contributor

@aglinxinyuan aglinxinyuan left a comment

Choose a reason for hiding this comment

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

LGTM!

@mengw15 mengw15 merged commit 4d1d51d into master Oct 12, 2024
@mengw15 mengw15 deleted the meng-monaco-upgrade branch October 12, 2024 19:01
Copy link
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

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

I have some questions regarding the changes, to help me debug the CI issue.

Comment on lines +10 to +14
parser: {
javascript: {
url: true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@mengw15 what's the purpose of this change? I found removing it has no effect.

Copy link
Contributor Author

@mengw15 mengw15 Oct 20, 2024

Choose a reason for hiding this comment

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

I was facing an issue with loading vscode.theme-default I remember. And I checked Monaco's example, below are some context:

"
// this is required for loading .wasm (and other) files. For context, see https://stackoverflow.com/a/75252098 and angular/angular-cli#24617
parser: {
javascript: {
url: true
}
}
"

"karma-jasmine-html-reporter": "1.7.0",
"nodecat": "2.0.0",
"nx": "18.1.3",
"nx": "18.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you changing nx version here?

Copy link
Contributor Author

@mengw15 mengw15 Oct 20, 2024

Choose a reason for hiding this comment

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

Xinyuan told me his branch passed all the checks before, so he think the issue is caused by nx, so we upgraded the version

Yicong-Huang added a commit that referenced this pull request Oct 22, 2024
…ion (#2954)

This PR fixes CI issues introduced in #2912. The symptom is frontend
test ci failed without failing the entire CI. There are a few reasons
underneath:

- **Alias dependency resolution issue.** The newly introduced
`Monaco-editor-wrapper` has peer dependency with alias
`@codingame/monaco-vscode-api@8.0.4`. Yarn@1 will install it as
`vscode@npm:@codingame/monaco-vscode-api@8.0.4` in the lock file.
However, during parsing the lock file, Nx reports cannot find the
package with name `npm:vscode@npm:@codingame/monaco-vscode-api` (note
for the `npm:` in the beginning).

> Solution (2 steps): 
> 1. Added resolution in package.json `monaco-editor:
npm:vscode@npm:@codingame/monaco-vscode-api` so that the generated lock
file has correct handle.
> 2. To be able to parse this added resolution field, Yarn@1 has to be
upgraded to Yarn@4.5.1.

- **multiple webpack resolution issue.** Our project uses a custom
webpack. Monaco also introduces their webpack dependency. There are
multiple webpacks, and the used version got resolved to be `@5.92.3`
which caused error when bundling frontend compile files.

> Solution: pin webpack version to use `@5.89.0` in resolution.

- **Nx not failing upon a failed execution.** In #2912, we also updated
the Nx from @18.1.3 to @18.2.0. The frontend test is failed to be
executed, but Nx reports success (did not return error).

> Solution: this seems to be a bug on Nx side. in release @18.2.0, a bug
was introduced so that errors reported from the project graph build
during the daemon process would not be rethrown, causing the main thread
to not fail properly. We updated it to version @20.0.3 and it is
confirmed to be solved. See
https://github.com/Texera/texera/actions/runs/11429004023 (it properly
failed the CI).


### Steps to migrate this PR:
Under `core` directory:
- You need to switch from Yarn@1 to Yarn@4.5.1: run `corepack enable &&
corepack prepare yarn@4.5.1 --activate && yarn --cwd gui set version
4.5.1`

Under `core/gui` directory:
- You can verify the Yarn version with `yarn --version`. It should show
4.5.1
- Remove all cached/dependency files: `rm -rf node_modules .angular`
- Reinstall with the upgraded Yarn: `yarn install`
PurelyBlank pushed a commit that referenced this pull request Dec 4, 2024
This PR upgrades the Monaco Language Client from the previous version to
8.8.3. It includes upgrades to related libraries to ensure compatibility
with the new version. The implementation has been adjusted to align with
the new API.

---------

Co-authored-by: linxinyuan <linxinyuan@gmail.com>
Co-authored-by: Xinyuan Lin <xinyual3@uci.edu>
PurelyBlank pushed a commit that referenced this pull request Dec 4, 2024
…ion (#2954)

This PR fixes CI issues introduced in #2912. The symptom is frontend
test ci failed without failing the entire CI. There are a few reasons
underneath:

- **Alias dependency resolution issue.** The newly introduced
`Monaco-editor-wrapper` has peer dependency with alias
`@codingame/monaco-vscode-api@8.0.4`. Yarn@1 will install it as
`vscode@npm:@codingame/monaco-vscode-api@8.0.4` in the lock file.
However, during parsing the lock file, Nx reports cannot find the
package with name `npm:vscode@npm:@codingame/monaco-vscode-api` (note
for the `npm:` in the beginning).

> Solution (2 steps): 
> 1. Added resolution in package.json `monaco-editor:
npm:vscode@npm:@codingame/monaco-vscode-api` so that the generated lock
file has correct handle.
> 2. To be able to parse this added resolution field, Yarn@1 has to be
upgraded to Yarn@4.5.1.

- **multiple webpack resolution issue.** Our project uses a custom
webpack. Monaco also introduces their webpack dependency. There are
multiple webpacks, and the used version got resolved to be `@5.92.3`
which caused error when bundling frontend compile files.

> Solution: pin webpack version to use `@5.89.0` in resolution.

- **Nx not failing upon a failed execution.** In #2912, we also updated
the Nx from @18.1.3 to @18.2.0. The frontend test is failed to be
executed, but Nx reports success (did not return error).

> Solution: this seems to be a bug on Nx side. in release @18.2.0, a bug
was introduced so that errors reported from the project graph build
during the daemon process would not be rethrown, causing the main thread
to not fail properly. We updated it to version @20.0.3 and it is
confirmed to be solved. See
https://github.com/Texera/texera/actions/runs/11429004023 (it properly
failed the CI).


### Steps to migrate this PR:
Under `core` directory:
- You need to switch from Yarn@1 to Yarn@4.5.1: run `corepack enable &&
corepack prepare yarn@4.5.1 --activate && yarn --cwd gui set version
4.5.1`

Under `core/gui` directory:
- You can verify the Yarn version with `yarn --version`. It should show
4.5.1
- Remove all cached/dependency files: `rm -rf node_modules .angular`
- Reinstall with the upgraded Yarn: `yarn install`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants