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

monaco: uplift to 1.72.3 #11787

Merged
merged 2 commits into from
Nov 10, 2022
Merged

monaco: uplift to 1.72.3 #11787

merged 2 commits into from
Nov 10, 2022

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Oct 19, 2022

What it does

The pull-request uplifts monaco to 1.72.2 (latest vscode tag).

image

image

How to test

Checklist:

  • confirm that syntax highlighting works for multiple languages
  • confirm that the outline-view is properly populated
  • confirm that problem markers work properly
  • confirm that language-features work properly (go to definition, call hierarchy, peek >, refactor)
  • confirm that quick fixes work
  • confirm that the quick input and quick open menu works
  • confirm that editor styling works
  • confirm that the new preferences work (ex: editor.stickyScroll.enabled)
  • confirm other editor or monaco related features work

Additional Information

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the monaco issues related to monaco label Oct 19, 2022
@vince-fugnitto vince-fugnitto self-assigned this Oct 19, 2022
@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch 5 times, most recently from 84a2a0e to 645262f Compare October 20, 2022 16:11
@planger
Copy link
Contributor

planger commented Oct 20, 2022

@vince-fugnitto I saw that this PR breaks the playwright tests. Please feel free to ping me anytime if we can help out adapting the page object library to make the tests work again.

@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch 4 times, most recently from 0555e3b to 72c65fd Compare October 24, 2022 15:18
@vince-fugnitto
Copy link
Member Author

@vince-fugnitto I saw that this PR breaks the playwright tests. Please feel free to ping me anytime if we can help out adapting the page object library to make the tests work again.

@planger thanks! You can take a look at them if you'd like, I was having a hard time actually debugging playwright.

@planger
Copy link
Contributor

planger commented Oct 25, 2022

@vince-fugnitto Thanks, we'll take a look! As many of us are currently on EclipseCon, I'm not sure we'll get to it this week though. To avoid blocking your PR, feel free to disable the failing test temporarily, and we'll open a PR at latest next week to fix and re-enable it.

Thanks!

@planger
Copy link
Contributor

planger commented Oct 28, 2022

@vince-fugnitto I've had a look at the failing test. It seems like Monaco now adds a "zero width non-joiner" character (‌ or \u200c) to the HTML text content. So the most direct way to resolve this is to replace the method replaceEditorSymbolsWithSpace(content: string) in examples/playwright/src/theia-text-editor.ts with the one below:

    protected replaceEditorSymbolsWithSpace(content: string): string | Promise<string | undefined> {
        // [ ] &nbsp; => \u00a0 -- NO-BREAK SPACE
        // [·] &middot; => \u00b7 -- MIDDLE DOT
        // [] &zwnj; => \u200c -- ZERO WIDTH NON-JOINER
        return content.replace(/[\u00a0\u00b7]/g, ' ').replace(/[\u200c]/g, '');
    }

@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch from 72c65fd to b0a7e95 Compare October 28, 2022 16:52
@vince-fugnitto vince-fugnitto marked this pull request as ready for review October 28, 2022 17:58
@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch from b0a7e95 to b821dc2 Compare November 1, 2022 13:12
@msujew msujew self-requested a review November 1, 2022 15:16
@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch 2 times, most recently from a7f7a0d to 562bd7e Compare November 7, 2022 13:55
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I've tried testing the monaco features and the added preferences in particular. Everything seems to work as expected 👍

I would propose to merge this PR rather sooner than later to catch potential issues before the next release.

@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch from 562bd7e to 7b9be65 Compare November 7, 2022 16:27
@vince-fugnitto
Copy link
Member Author

I would propose to merge this PR rather sooner than later to catch potential issues before the next release.

Thank you for the review!

I performed an official release of @theia/monaco-editor-core@1.72.3 and filed a CQ for it with the modifications to license and third-party-notices the foundation asked for.

@vince-fugnitto
Copy link
Member Author

The latest CQ has been approved 👍

@vince-fugnitto vince-fugnitto changed the title monaco: uplift to 1.72.2 monaco: uplift to 1.72.3 Nov 8, 2022
@vince-fugnitto
Copy link
Member Author

As mentioned in the dev meeting, I'll merge the pull-request tomorrow and we have the rest of the month to identify any potential regressions.

The commit upgrades monaco to `v1.72.3`.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-upgrade-1.7.x branch from 7b9be65 to 851ffab Compare November 8, 2022 16:57
@vince-fugnitto
Copy link
Member Author

The test seems to fail due to quickInputList.ts#getAriaLabel, where the recently used seperator is added to the aria-label.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto merged commit e48b1fd into master Nov 10, 2022
@vince-fugnitto vince-fugnitto deleted the vf/monaco-upgrade-1.7.x branch November 10, 2022 16:00
@github-actions github-actions bot added this to the 1.32.0 milestone Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants