Skip to content

Bump monaco-editor from 0.30.1 to 0.52.0#9618

Merged
ananzh merged 5 commits intoopensearch-project:mainfrom
ananzh:bump-monaco-2
Apr 1, 2025
Merged

Bump monaco-editor from 0.30.1 to 0.52.0#9618
ananzh merged 5 commits intoopensearch-project:mainfrom
ananzh:bump-monaco-2

Conversation

@ananzh
Copy link
Copy Markdown
Member

@ananzh ananzh commented Mar 27, 2025

Note: Need to merge #9497 first and rebase.

Description

Bump monaco-editor from 0.30.1 to 0.52.0. Several key API changes that required updates to the codebase:

CodeEditor Component Changes:

  • The react-monaco-editor component API changed, requiring explicit passing of properties like defaultValue, overrideServices, and default values for width and options.
  • This was necessary to maintain compatibility with existing snapshot tests.

Worker Implementation Changes:

  • The worker architecture was significantly redesigned in monaco-editor 0.52.0. The simplified worker implementations with basic message handlers are necessary placeholders that establish the required worker interface
  • The actual language services are now loaded differently or on-demand in the new version

MonacoEnvironment Configuration Changes:

  • The getWorker function signature changed from (module: string, languageId: string) to (workerId: string, label: string)
  • The addition of a fallback worker ensures robustness when monaco-editor requests a worker for a language or service without a specific implementation

Import Path Changes:

  • Many import paths now include /browser/ in their structure, reflecting monaco-editor's internal reorganization
  • The updates properly adapt to monaco-editor's new architecture for workers, language services, and component APIs.

All the issues while bumping to 0.52.0 have been documented in #9573

Issues Resolved

#9573

Screenshot

eiditor-0.52.0-2.mp4

Testing the changes

Changelog

  • breaking: Bump monaco-editor from 0.30.1 to 0.52.0

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 28, 2025
opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 28, 2025
Issue Resolved:
opensearch-project#9573

Signed-off-by: Anan <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
opensearch-changeset-bot bot added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 31, 2025
angle943
angle943 previously approved these changes Mar 31, 2025
window.MonacoEnvironment = {
getWorker: (module: string, languageId: string) => {
const workerSrc = getWorker(languageId);
getWorker: (workerId: string, label: string) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: for developers i can safely assume that if i pass an id to a function getWorker it would be assumed that is the worker ID so it's a little bit redundant.

however from what i can tell that the workerId is actually not used so to be i think i preferred the discarded parameter convention of using an underscore _ to indicate that we are not using the id. this will indicate to developers that we actually don't care for the worker Id

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good point. will change.

Comment thread packages/osd-monaco/src/monaco.d.ts Outdated
}

declare module 'monaco-editor/min/vs/base/browser/ui/codicons/codicon/codicon.css' {
const content: any;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are there better types than any?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The CSS imports are used with a side-effect import syntax (import 'monaco-editor/min/vs/editor/editor.main.css';), which means the content isn't actually assigned to a variable. The type doesn't matter much in this case since it's just for allowing the import to work without TypeScript errors.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But I could update it to string.

// This is a simplified worker that doesn't rely on monaco-editor's internal modules
// It just creates a basic worker that can be loaded by monaco

self.onmessage = () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

q: do we need this file anymore if it just does nothing anyways?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The monaco-editor 0.52.0 has a redesigned worker architecture that requires these placeholder files to establish the worker interface. Even though they don't do much (just have a basic onmessage handler), they're necessary for the new architecture to function properly.

json.worker.ts is the actual web worker entry point that gets compiled by webpack into a standalone JavaScript file that can be loaded as a web worker by raw-loader.

import jsonWorkerSrc from '!!raw-loader!../../target/public/json.editor.worker.js';

This is the file that Monaco will load as a web worker. In the new Monaco architecture (0.52.0), this file needs to have a specific structure with an onmessage handler, even if it's minimal.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tbh i'm not really sure. what goes into play here. or how this worker gets used and it's life cycle

nit: would it be worth to consolidate to a dummy worker file? since the json and xjson are identical now?

worker.initialize((ctx: any, createData: any) => {
return new XJsonWorker(ctx);
});
// Basic initialization
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same q: could we just delete this file if it does nothing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same as above.

For the XJSON worker, there's a more complex setup with the actual implementation in xjson_worker.ts and a proxy service that communicates with it, but the xjson.worker.ts file is still needed as the entry point for the worker.

xjson_worker.ts contains the actual implementation of the worker functionality (the XJsonWorker class). This class defines the methods and logic that will be executed inside the worker (what does the work).

xjson.worker.ts is the entry point for the web worker (what gets loaded).

The flow is similar to json. The difference is that after Monaco creates the worker from the compiled xjson.worker.ts (use raw-loader in xjson/language.ts). The worker uses the implementation from xjson_worker.ts to do the actual work.

Anyway, these files are needed.

[Sun Mar 7 20:58:27 2004] [info] [client xx.xx.xx.xx] (104)Connection reset by peer: client stopped connection before send body completed
[Sun Mar 7 21:16:17 2004] [error] [client xx.xx.xx.xx] File does not exist: /home/httpd/twiki/view/Main/WebHome
"
width="100%"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this because the css import? do we know the full usage of the monaco editor in the app. could we check this repo and opensearch project. if the width is no longer 100% for this snapshot than i wonder if the default got changed which might have unintended consequences to plugin teams

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In the current version (0.58.0) of react-monaco-editor, the MonacoEditor component is implemented as a functional component with default props defined using destructuring in the function parameters:

function MonacoEditor(_a) {
    var _b = _a.width, width = _b === void 0 ? "100%" : _b, _c = _a.height, height = _c === void 0 ? "100%" : _c, /* other props */
    // ...
}

This means that if the width prop is not provided, it defaults to "100%". However, when using Enzyme's shallow rendering in the test, these default props are not included in the snapshot because they're handled internally by the component.

In the previous version (0.27.0) of react-monaco-editor, the MonacoEditor component was likely implemented as a class component with default props defined using the static defaultProps property:

class MonacoEditor extends React.Component {
    // ...
}

MonacoEditor.defaultProps = {
    width: "100%",
    height: "100%",
    // other default props
};

When using Enzyme's shallow rendering with a class component that has static defaultProps, these default props are included in the snapshot even if they're not explicitly passed to the component.

The actual behavior of the component hasn't changed - it still uses a default width of "100%" if no width is provided - but the way the default props are handled in the test snapshot has changed due to the different implementation of the component.

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh merged commit 8f57da4 into opensearch-project:main Apr 1, 2025
74 of 76 checks passed
@ananzh ananzh mentioned this pull request Apr 1, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants