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

Add json worker to monaco editor #3424

Merged

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Feb 14, 2023

Signed-off-by: suzhou [email protected]

Description

  1. Add a new entry in webpack.config.js to build json.editor.worker.js.

Issues Resolved

#3132

Check List

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

@SuZhou-Joe SuZhou-Joe requested a review from a team as a code owner February 14, 2023 11:46
@SuZhou-Joe
Copy link
Member Author

#3121

@SuZhou-Joe
Copy link
Member Author

#3132

@AMoo-Miki
Copy link
Collaborator

@SuZhou-Joe Can you please look into the test failures?

Signed-off-by: suzhou <[email protected]>
@SuZhou-Joe SuZhou-Joe changed the title Feature/alias monaco next Add json worker to monaco editor Feb 15, 2023
@SuZhou-Joe
Copy link
Member Author

@SuZhou-Joe Can you please look into the test failures?

Yes, the monaco implementation has side effect and can not be import twice in a repo, change to only add a json worker.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2023

Codecov Report

Merging #3424 (e86afbf) into main (02e0bc1) will decrease coverage by 0.06%.
The diff coverage is 72.72%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3424      +/-   ##
==========================================
- Coverage   66.45%   66.40%   -0.06%     
==========================================
  Files        3205     3208       +3     
  Lines       61562    61580      +18     
  Branches     9497     9498       +1     
==========================================
- Hits        40912    40891      -21     
- Misses      18353    18411      +58     
+ Partials     2297     2278      -19     
Flag Coverage Δ
Linux 66.40% <72.72%> (+<0.01%) ⬆️
Windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/osd-monaco/src/monaco_environment.ts 33.33% <33.33%> (ø)
packages/osd-monaco/src/worker_store.ts 75.00% <75.00%> (ø)
packages/osd-monaco/src/index.ts 100.00% <100.00%> (ø)
packages/osd-monaco/src/json/index.ts 100.00% <100.00%> (ø)
packages/osd-monaco/src/xjson/language.ts 51.42% <100.00%> (+5.48%) ⬆️
src/dev/build/lib/get_build_number.ts 57.14% <0.00%> (-42.86%) ⬇️
src/setup_node_env/harden/child_process.js 38.46% <0.00%> (-38.47%) ⬇️
packages/osd-cross-platform/src/path.ts 51.21% <0.00%> (-34.15%) ⬇️
...ges/osd-apm-config-loader/src/config.test.mocks.ts 91.30% <0.00%> (-8.70%) ⬇️
src/dev/build/lib/config.ts 79.41% <0.00%> (-5.89%) ⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Feb 15, 2023

Build and test / Run backwards compatibility tests (osd-2.1.0) (pull_request) failed by flaky test and passed on this flow d80b42e .

@AMoo-Miki
Copy link
Collaborator

Question: Since this is working on top of monaco-next, how will it impact plugins that continue to use monaco?

Also, can you please rebase on top of main and resolve conflicts?

@SuZhou-Joe
Copy link
Member Author

SuZhou-Joe commented Feb 17, 2023

Question: Since this is working on top of monaco-next, how will it impact plugins that continue to use monaco?

Also, can you please rebase on top of main and resolve conflicts?

Yes, after a quick try to add "@osd/monaco-next", we found some issues happening on other plugins, which indicate the monaco implementation has some side effect. So the PR now only adds a json worker and move the language implementation into IM plugin until the 0.20.0 research and migration is complete.

@SuZhou-Joe SuZhou-Joe requested review from joshuarrrr and ananzh and removed request for joshuarrrr February 21, 2023 02:21
@SuZhou-Joe
Copy link
Member Author

@SuZhou-Joe Does this PR obsolete #3133? If so, can you close that one?

Sure, done for this.

@ananzh
Copy link
Member

ananzh commented Feb 22, 2023

hey @SuZhou-Joe ,

Today is code freeze of 2.6. We have to move this to 2.7.

@ananzh ananzh added v2.7.0 and removed v2.6.0 labels Feb 22, 2023
@SuZhou-Joe SuZhou-Joe requested review from joshuarrrr and ananzh and removed request for ananzh and joshuarrrr March 3, 2023 02:25
@ashwin-pc ashwin-pc self-assigned this Mar 7, 2023
@ananzh
Copy link
Member

ananzh commented Mar 7, 2023

The pull request adds a new plugin called monaco_editor that includes the necessary code to embed the Monaco editor into OpenSearch-Dashboards. When I reviewed it, I am stuck at understanding 1)whether the changes involve updating various services, and UI templates to support the new editor, as well as adding new CSS styles and configuration files. 2) how to test thoroughly.

@SuZhou-Joe
Copy link
Member Author

The pull request adds a new plugin called monaco_editor that includes the necessary code to embed the Monaco editor into OpenSearch-Dashboards. When I reviewed it, I am stuck at understanding 1)whether the changes involve updating various services, and UI templates to support the new editor, as well as adding new CSS styles and configuration files. 2) how to test thoroughly.

  1. Nope, it only involves a web worker to analyze the content, exactly the same as xjson worker.
  2. Monaco itself has done testing on this worker.

@ashwin-pc
Copy link
Member

@SuZhou-Joe The code change looks fine to me, however im struggling to reproduce the original and modified screens that you are showing. Are there any steps i can use to validate the change that you are making?

There are two instances of this editor being used in OSD:

  1. Timeline visualizations
  2. Vega lite spec inspector

In both instances, i do not see the syntax highlighting that this change is supposed to introduce.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This change looks good to me.

Note: Just want to call out for future reference that this PR alone does not add JSON support to the monaco editor in OSD, it only adds a json worker that the ISM plugin will use to load the json language support code it has within the plugin. This monaco editor in OSD will need a rework to properly support other languages, and this PR does not change that. Approving to unblock ISM json features.

@joshuarrrr joshuarrrr merged commit 725a2a1 into opensearch-project:main Mar 16, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 16, 2023
* feat: add json worker and language implement in dashboards

Signed-off-by: suzhou <[email protected]>

---------

Signed-off-by: suzhou <[email protected]>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit 725a2a1)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
abbyhu2000 pushed a commit that referenced this pull request Mar 22, 2023
* Add json worker to monaco editor (#3424)

* feat: add json worker and language implement in dashboards

Signed-off-by: suzhou <[email protected]>

---------

Signed-off-by: suzhou <[email protected]>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
(cherry picked from commit 725a2a1)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

* Add changelog

Signed-off-by: Josh Romero <[email protected]>

---------

Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Josh Romero <[email protected]>
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Mar 22, 2023
* feat: add json worker and language implement in dashboards

Signed-off-by: suzhou <[email protected]>

---------

Signed-off-by: suzhou <[email protected]>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
* feat: add json worker and language implement in dashboards

Signed-off-by: suzhou <[email protected]>

---------

Signed-off-by: suzhou <[email protected]>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
* feat: add json worker and language implement in dashboards

Signed-off-by: suzhou <[email protected]>

---------

Signed-off-by: suzhou <[email protected]>
Co-authored-by: Anan Zhuang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
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.

6 participants