add monaco to kbn/ui-shared-deps and load required features for all uses #58075

spalger · 2020-02-20T01:39:03Z

Once we merged the @kbn/optimizer we broke the syntax highlighting in canvas because the monaco editor is a stateful module that we weren't sharing via @kbn/ui-shared-deps. This moves it into the shared deps package so that all bundles/plugins can use it and so that it is bundled once.

To use the monaco editor code should now use:

import { monaco } from '@kbn/ui-shared-deps/monaco';

This monaco instance is the one populated with features by packages/kbn-ui-shared-deps/monaco.ts and is used everywhere. The monaco-editor/esm/vs/editor/editor.api is still enabled for react-monaco-editor but should not be used by applications.

The interface of the <CodeEditor /> from kibana-react was also tweaked as to not expose the monaco editor instance to handlers. The argument should always be the same instance accessible via the import, so that is now asserted whenever possible and consumers that need access to the monaco instance should import it instead. (this change was primarily to deal with typeof monaco instances that made less sense now that we're standardized on the monaco vs monacoEditor naming everywhere)

…itor-syntax-highlighting

spalger · 2020-02-20T02:21:57Z

src/plugins/kibana_react/public/code_editor/code_editor.tsx

+    if (__monaco !== monaco) {
+      throw new Error('react-monaco-editor is using a different version of monaco');
+    }


It seems the only reason to expose the monaco instance to handlers like editorDidMount() is so consumers can get access to the version of monaco being used by react-monaco-editor. In our case this version should always be the same as the version imported from @kbn/ui-shared-deps/monaco, so I updated this and the signature for these handlers to ensure we keep a single source of truth.

Excellent, yeah this makes sense to me

…itor-syntax-highlighting

poffdeluxe

Changes look good -- I think one thing we might have to check, however, is if there's any further work that needs to be done to get the jest tests all working. We tell jest to transform the monaco editor so the imports work correctly: https://github.com/elastic/kibana/blob/master/src/dev/jest/config.js#L90

spalger · 2020-02-20T16:47:32Z

I think one thing we might have to check, however, is if there's any further work that needs to be done to get the jest tests all working

Jest should still work because the import of @kbn/ui-shared-deps/monaco still imports from node_modules/monaco-editor which is when that transform kicks in.

kibanamachine · 2020-02-20T17:43:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dcb84be

History

💔 Build #27789 failed af9a7e0
💔 Build #27784 failed 95e1779

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

elasticmachine · 2020-02-20T17:46:14Z

Pinging @elastic/kibana-operations (Team:Operations)

lukasolson

AppArch code changes LGTM

tylersmalley · 2020-02-20T19:54:38Z

src/plugins/kibana_react/public/code_editor/code_editor.tsx

    });

    // Register the theme
    monaco.editor.defineTheme('euiColors', this.props.useDarkTheme ? DARK_THEME : LIGHT_THEME);


Should this be set on the default package? Or do we still want to leave it up to the implementation to import the colors?

Not a blocker to this PR since this is the current implementation.

We should avoid putting any code into the @kbn/ui-shared-deps that isn't directly from npm as that module is only built on bootstrap. So I'm going to say we should leave it here.

…ses (elastic#58075) * add monaco to kbn/ui-shared-deps and load required features for all uses * forgot to save a change * remove unused imports * include a cache buster to counteract issue elastic#58077 * include monaco.ts in ts project

… all uses (#58075) (#58167) * add monaco to kbn/ui-shared-deps and load required features for all uses (#58075) * add monaco to kbn/ui-shared-deps and load required features for all uses * forgot to save a change * remove unused imports * include a cache buster to counteract issue #58077 * include monaco.ts in ts project * update yarn.lock

spalger · 2020-02-20T23:51:28Z

7.x/7.7: 23e3ce7

…-out-of-legacy * 'master' of github.com:elastic/kibana: (109 commits) document difference between log record formats (elastic#57798) Expose elasticsearch config schema (elastic#57655) [ui/agg_response/tabify] update types for search/expressions/build_tabular_inspector_data.ts (elastic#58130) [SIEM] Cleans Cypress tests code (elastic#58134) fix: 🐛 make dev server Storybook builds work again (elastic#58188) Prevent core savedObjects plugin from being overridden (elastic#58193) Expose serverBasePath on client-side (elastic#58070) Fix legend sizing on area charts (elastic#58083) Drilldown plugin (elastic#58097) [skip-ci] Fix broken links to saved objects APIs in MIGRATION.md (elastic#58033) [ML] New Platform server shim: update datafeed routes (elastic#57739) Add flag for building static storybook site (elastic#58050) add monaco to kbn/ui-shared-deps and load required features for all uses (elastic#58075) [SIEM] Let us try out code owners for a little while and see what happens Add throttle param to Alerting readme (elastic#57609) [NP] Move ui/saved_objects to NP (elastic#57452) [Logs UI] Fix column reordering in settings page (elastic#58104) Fix browser date format (elastic#57714) Add filter for ILM phase to Index Management (revert elastic#45486) (elastic#57402) Clarify Precision function in Timelion Kibana (elastic#58031) ... # Conflicts: # x-pack/.i18nrc.json

sulemanof · 2020-02-25T15:46:21Z

Hey @spalger
Seems this broke up the existing behavior:

this is how it looks in the Timelion editor:

canvas:

I did yarn kbn clean & yarn kbn bootstrap before starting kibana.

spalger · 2020-02-26T15:32:33Z

Fixed the bug you mentioned in #58520, thanks!

spalger added 5 commits February 19, 2020 18:33

add monaco to kbn/ui-shared-deps and load required features for all uses

95e1779

forgot to save a change

f2ec2be

remove unused imports

e5126a4

include a cache buster to counteract issue elastic#58077

3450f99

Merge branch 'master' of github.com:elastic/kibana into fix/canvas-ed…

af9a7e0

…itor-syntax-highlighting

spalger commented Feb 20, 2020

View reviewed changes

spalger added 3 commits February 19, 2020 22:01

Merge branch 'master' of github.com:elastic/kibana into fix/canvas-ed…

62612dd

…itor-syntax-highlighting

Merge branch 'master' of github.com:elastic/kibana into fix/canvas-ed…

34a7027

…itor-syntax-highlighting

include monaco.ts in ts project

dcb84be

poffdeluxe approved these changes Feb 20, 2020

View reviewed changes

spalger marked this pull request as ready for review February 20, 2020 17:45

spalger requested review from a team as code owners February 20, 2020 17:45

spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Kibana-Operations Team v7.7.0 v8.0.0 labels Feb 20, 2020

spalger requested a review from poffdeluxe February 20, 2020 17:46

lukasolson approved these changes Feb 20, 2020

View reviewed changes

tylersmalley reviewed Feb 20, 2020

View reviewed changes

poffdeluxe approved these changes Feb 20, 2020

View reviewed changes

tylersmalley approved these changes Feb 20, 2020

View reviewed changes

spalger merged commit 4aedf2b into elastic:master Feb 20, 2020

spalger mentioned this pull request Feb 20, 2020

[7.x] add monaco to kbn/ui-shared-deps and load required features for all uses (#58075) #58167

Merged

spalger deleted the fix/canvas-editor-syntax-highlighting branch February 20, 2020 23:51

spalger mentioned this pull request Feb 25, 2020

[kbn/ui-shared-deps] load base css file #58520

Merged

add monaco to kbn/ui-shared-deps and load required features for all uses #58075

add monaco to kbn/ui-shared-deps and load required features for all uses #58075

Uh oh!

Conversation

spalger commented Feb 20, 2020

Uh oh!

spalger Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

poffdeluxe Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

poffdeluxe left a comment

Choose a reason for hiding this comment

Uh oh!

spalger commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Feb 20, 2020

💚 Build Succeeded

History

Uh oh!

elasticmachine commented Feb 20, 2020

Uh oh!

lukasolson left a comment

Choose a reason for hiding this comment

Uh oh!

tylersmalley Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

spalger Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

spalger commented Feb 20, 2020

Uh oh!

sulemanof commented Feb 25, 2020

Uh oh!

spalger commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

spalger commented Feb 20, 2020 •

edited

Loading

tylersmalley Feb 20, 2020 •

edited

Loading