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

feat: add json worker and language implement in dashboards #3133

Closed
wants to merge 11 commits into from

Conversation

SuZhou-Joe
Copy link
Member

@SuZhou-Joe SuZhou-Joe commented Dec 23, 2022

Signed-off-by: suzhou [email protected]

Description

  1. Add a new entry in webpack.config.js to build json.editor.worker.js.
  2. Add json language support for monaco instance.

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
Copy link
Member Author

#3132

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Merging #3133 (095e493) into main (c259952) will increase coverage by 0.04%.
The diff coverage is 60.97%.

@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
+ Coverage   66.51%   66.56%   +0.04%     
==========================================
  Files        3203     3205       +2     
  Lines       61322    61362      +40     
  Branches     9452     9458       +6     
==========================================
+ Hits        40786    40843      +57     
+ Misses      18275    18262      -13     
+ Partials     2261     2257       -4     
Flag Coverage Δ
Linux 66.50% <60.97%> (-0.01%) ⬇️
Windows 66.51% <60.97%> (?)

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

Impacted Files Coverage Δ
src/plugins/opensearch_ui_shared/public/index.ts 0.00% <ø> (ø)
...s_options/json_editor_with_diagnostics_options.tsx 18.75% <18.75%> (ø)
packages/osd-monaco/src/xjson/language.ts 48.78% <60.00%> (+2.83%) ⬆️
packages/osd-monaco/src/json/index.ts 94.73% <94.73%> (ø)
packages/osd-monaco/src/index.ts 100.00% <100.00%> (ø)
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 51.31% <0.00%> (-1.32%) ⬇️
...ic/application/models/sense_editor/sense_editor.ts 65.77% <0.00%> (+1.77%) ⬆️
src/dev/build/lib/config.ts 85.29% <0.00%> (+5.88%) ⬆️
...ges/osd-apm-config-loader/src/config.test.mocks.ts 100.00% <0.00%> (+8.69%) ⬆️
... and 3 more

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

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

I wouldn't expect that any changes are necessary within the xjson directory, and that the implementation of json support would more closely mirror that of xjson.

CHANGELOG.md Outdated Show resolved Hide resolved
packages/osd-monaco/src/index.ts Outdated Show resolved Hide resolved
packages/osd-monaco/src/xjson/language.ts Show resolved Hide resolved
@joshuarrrr
Copy link
Member

@SuZhou-Joe It looks like there is a failing test:

Summary of all failing tests
FAIL src/plugins/vis_type_timeline/public/components/timeline_expression_input_helpers.test.ts
● Test suite failed to run

ReferenceError: monaco is not defined

  34 |  * */
  35 | // @ts-ignore
> 36 | import * as mode from 'monaco-editor/esm/vs/language/json/jsonMode.js'; // eslint-disable-line
     | ^
  37 | import { monaco } from '../monaco';
  38 |
  39 | const Emitter = monaco.Emitter;

  at Object.<anonymous> (node_modules/monaco-editor/esm/vs/language/json/languageFeatures.js:7:11)
  at Object.<anonymous> (node_modules/monaco-editor/esm/vs/language/json/jsonMode.js:7:1)
  at Object.<anonymous> (packages/osd-monaco/src/json/index.ts:36:1)
  at Object.<anonymous> (packages/osd-monaco/src/index.ts:30:1)
  at Object.<anonymous> (src/plugins/vis_type_timeline/public/components/timeline_expression_input_helpers.ts:33:1)
  at Object.<anonymous> (src/plugins/vis_type_timeline/public/components/timeline_expression_input_helpers.test.ts:31:1)

Test Suites: 1 failed, 2 skipped, 1503 passed, 1504 of 1506 total
Tests: 36 skipped, 9 todo, 11372 passed, 11417 total
Snapshots: 2506 passed, 2506 total
Time: 2923.118 s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 1.

@SuZhou-Joe
Copy link
Member Author

Yes, let me change the order of importing.

@SuZhou-Joe
Copy link
Member Author

Windows

Fixed

@SuZhou-Joe SuZhou-Joe requested review from joshuarrrr and kavilla and removed request for joshuarrrr and kavilla December 31, 2022 11:53
@joshuarrrr joshuarrrr added v2.5.0 'Issues and PRs related to version v2.5.0' backport 2.x labels Jan 5, 2023
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

Can we add json in README? For example,

Includes json language support. 

and add an example?

packages/osd-monaco/src/json/index.ts Outdated Show resolved Hide resolved
@SuZhou-Joe
Copy link
Member Author

Can we add json in README? For example,

Includes json language support. 

and add an example?

Done for example and add that in README.

@SuZhou-Joe SuZhou-Joe requested review from ananzh and joshuarrrr and removed request for joshuarrrr and ananzh January 9, 2023 08:51
Signed-off-by: suzhou <[email protected]>
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

I'm concerned about the comment justifying a workaround for a bug in monaco-editor < v0.20.0. Why aren't we just bumping the dependency instead?

CHANGELOG.md Outdated Show resolved Hide resolved
packages/osd-monaco/src/json/index.ts Outdated Show resolved Hide resolved
Comment on lines +31 to +34
/**
* monaco-editor under v0.20.0 has the bug that use the global monaco uncorrectly in ESM mode. https://github.com/microsoft/monaco-editor/issues/1974
* copy the language implement and import by our self.
* */
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we just bumping the version of monaco instead? There's no reason to introduce the maintenance burden of this code if it is no longer needed.

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Jan 10, 2023

Choose a reason for hiding this comment

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

It may contain some break changes if we bump that. We can safely delete this file to use the implementation of Monaco's once we bump the version to 0.20.x in the future. So there is no maintenance burden actually.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@SuZhou-Joe I remembered that we already looked into the potential breaking changes - doing the upgrade would be preferable and worthwhile compared to adding this type of fix to the code base. #3097 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, agreed. But I am new to index management and even newer to opensearch dashboards. Doing such bumping is a little bit dangerous. Should I bump that in this PR or raise another one?

@SuZhou-Joe SuZhou-Joe requested review from joshuarrrr and removed request for ananzh January 10, 2023 07:49
@abbyhu2000 abbyhu2000 removed backport 2.x v2.5.0 'Issues and PRs related to version v2.5.0' backport 2.5 labels Jan 10, 2023
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Let's at least investigate whether we can bump to 0.20.0 of monaco-editor to avoid introducing workarounds for upstream bugs.

A quick code search shows no usage of getConfiguration, onCompositionStart, onCompositionEnd, onDidPaste, or WorkspaceEdit.edits.

We've removed the 2.5.0 label, because this change is unlikely to meet today's code freeze deadline.

@SuZhou-Joe
Copy link
Member Author

Let's at least investigate whether we can bump to 0.20.0 of monaco-editor to avoid introducing workarounds for upstream bugs.

A quick code search shows no usage of getConfiguration, onCompositionStart, onCompositionEnd, onDidPaste, or WorkspaceEdit.edits.

We've removed the 2.5.0 label, because this change is unlikely to meet today's code freeze deadline.

Actually I don't know the way to try or investigate if we can do that, should I just bump that or raise another PR to bump?

@SuZhou-Joe
Copy link
Member Author

Hi, is there any update on this PR? It's a dependency of our v2.6 release on Index management plugin.

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.

7 participants