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

[ES UI Shared] Monaco XJSON #67485

Merged
merged 33 commits into from
Jun 4, 2020
Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented May 27, 2020

Summary

Add XJSON mode support for Monaco. The intention is that this be used inside of Watcher, ML and any other new editors that we build so that we are able to use Monaco instead of Ace.

How to test

  1. Start Kibana
  2. Go to Painless Lab (this has been hacked to be an XJSON mode editor temporarily)
  3. Paste the following XJSON into the editor:
{
  "test": """test""",
  "script": """ while if else true """,
  "test": "sweet!",
  "else": true,
  "canBe": false,
  "object": {
    "inside": {
      "is": "another"
    }
  }
}

The XJSON should be highlighted correctly and indicate that there is a duplicate key "test".

Owners of specific sections

  • Note to ops, created a new package called @kbn/langs. In future I would like to put the grammar parsers for Console here too. This is similar to the @kbn/interpreter folder which is apparently going to be removed before (8.0.0) CC (@ppisljar). The alternative is that we put this inside es-ui-shared too. Any alternate suggestions welcome of course. I was thinking we can also put SQL, EQL and any other parsers we build here (if we build them).
  • Note to ML (@darnautov) please test the painless lab as described above, and test that your previous use of useXJsonMode works as expected

TODO

  • Implement an analogous XJSON mode hook for use with monaco.
  • Revert the dummy Painless Lab integration

Screenshots

Screenshot 2020-05-27 at 16 07 42

Additional Context

See this issue: #57435

@jloleysens jloleysens added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels May 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens jloleysens marked this pull request as ready for review May 28, 2020 13:57
@jloleysens jloleysens requested review from a team as code owners May 28, 2020 13:57
@jloleysens
Copy link
Contributor Author

@spalger I've created a new package @kbn/monaco. This has a file monaco.ts that is directly imported and bundled into @kbn/ui-shared-deps as before. Let me know if this is out of order regarding what we had discussed.

@jloleysens jloleysens requested a review from a team May 29, 2020 18:25
@jloleysens jloleysens requested a review from a team as a code owner May 29, 2020 18:25
@spalger
Copy link
Contributor

spalger commented May 30, 2020

Summary of changes I made:

  • bundle the worker with webpack on bootstrap
  • don't bundle the rest of @kbn/monaco, @kbn/ui-shares-deps creates the final bundle
  • move all source files into a src directory so we can import the worker bundle from src and target with the same relative path
  • transpile source from TS to JS on bootstrap
  • reference @kbn/monaco The same way we reference other modules shared via @kbn/ui-shared-deps
  • use the module migration eslint rule to rewrite @kbn/ui-shared-deps/monaco imports to @kbn/monaco and disallow imports for monaco-editor
  • use the shared @kbn/babel-preset
  • added a basic read me to the root of the package

Hope you don't mind me jumping in and applying these changes directly!

@spalger
Copy link
Contributor

spalger commented May 31, 2020

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

Summary of changes I made:

  • bundle the worker with webpack on bootstrap
  • don't bundle the rest of @kbn/monaco, @kbn/ui-shares-deps creates the final bundle
  • move all source files into a src directory so we can import the worker bundle from src and target with the same relative path
  • transpile source from TS to JS on bootstrap
  • reference @kbn/monaco The same way we reference other modules shared via @kbn/ui-shared-deps
  • use the module migration eslint rule to rewrite @kbn/ui-shared-deps/monaco imports to @kbn/monaco and disallow imports for monaco-editor
  • use the shared @kbn/babel-preset
  • added a basic read me to the root of the package

Hope you don't mind me jumping in and applying these changes directly!

Thanks a ton for sorting out the build and ops concerns @spalger ! This looks really great!

I was thinking the worker would change very infrequently, but I think building at bootstrap is so cheap to do so happy with that change too 👍

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Canvas changes look good to me

@kertal
Copy link
Member

kertal commented Jun 2, 2020

@elasticmachine merge upstream

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

KibanaApp Timelion usage LGTM, but in Painless lab I don't get the duplicate key message.

@jloleysens
Copy link
Contributor Author

Thanks for the reviews all!

KibanaApp Timelion usage LGTM, but in Painless lab I don't get the duplicate key message.

Looks like there was a minor issue with the worker bundle. Just fixed it, would you mind to taking another look @kertal @cjcenizal ?

I saw that it was erroring; this commit ad9f4d2 fixed the grammar parsing.

@kertal kertal self-requested a review June 2, 2020 16:38
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Kibana App owned Code LGTM, tested locally with Chrome, last issue I reported is fixed, the duplicate JSON property is indicated 👍

import { shallow } from 'enzyme';

// disabled because this is a test, but also it seems we shouldn't need this?
/* eslint-disable-next-line @kbn/eslint/module_migration */
import 'monaco-editor/esm/vs/basic-languages/html/html.contribution.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if this is something we should add to @kbn/monaco rather than importing it here? All other exclusions to this rule are in @kbn/monaco itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not sure. @poffdeluxe Do you have any guidance for us on this?

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It's extremely valuable for the ML and Transforms plugins.
monaco setup code LGTM, but some basic unit tests for tokenizers would be nice to have as it involves regexps.
nit: I've observed a bunch of commented code inside of painless_lab, you probably haven't finished with the clean up yet

@jloleysens
Copy link
Contributor Author

Thanks for reviewing @darnautov !

nit: I've observed a bunch of commented code inside of painless_lab, you probably haven't finished with the clean up yet

Yep! I will need to revert all painless commits before merging. Good point about the unit tests!

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested locally and looked through the code. Code organization generally made sense to me (the README helped, thanks!) though I'm not familiar with how Monaco works so not the best judge of code quality in this case. :)

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jloleysens
Copy link
Contributor Author

I am going to merge this work, any other issues can be addressed in follow up PRs

@jloleysens jloleysens merged commit 1346b15 into elastic:master Jun 4, 2020
@jloleysens jloleysens deleted the monaco-xjson branch June 4, 2020 11:43
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 4, 2020
* First iteration of xjson in monaco

* Throwaway implementation in painless lab - THIS MUST BE REVERTED

* WiP on build process for new kbn-lang package

* new @kbn/langs package and update ui-shared-deps

* Update jest config for new work files

* Update painless lab -- REVERT THIS COMMIT

* Create shared useXJson mode hook

* Final update to using the new shared useXJsonMode hook -- REVERT

* Created @kbn/monaco and share through shared deps

* always access monaco through `@kbn/monaco`

* use path.resolve to create path

* add basic readme

* remove console.log call

* remove typescript support from ui-shared-deps webpack config

* use `@kbn/babel-preset`

* include the monaco styles in the kbn-ui-shared-deps

* sort package.json

* build worker at bootstrap rather than commiting to repo

* only build worker, don't pre-bundle monaco

* fix type check errors

* remove section from readme about committed dist

* keep editor.worker.js postfix

* forgot to save update to import

* license package as apache-2.0

* Added regenerator runtime for worker bundle

* revert changes to painless lab

Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 4, 2020
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
jloleysens added a commit that referenced this pull request Jun 4, 2020
* First iteration of xjson in monaco

* Throwaway implementation in painless lab - THIS MUST BE REVERTED

* WiP on build process for new kbn-lang package

* new @kbn/langs package and update ui-shared-deps

* Update jest config for new work files

* Update painless lab -- REVERT THIS COMMIT

* Create shared useXJson mode hook

* Final update to using the new shared useXJsonMode hook -- REVERT

* Created @kbn/monaco and share through shared deps

* always access monaco through `@kbn/monaco`

* use path.resolve to create path

* add basic readme

* remove console.log call

* remove typescript support from ui-shared-deps webpack config

* use `@kbn/babel-preset`

* include the monaco styles in the kbn-ui-shared-deps

* sort package.json

* build worker at bootstrap rather than commiting to repo

* only build worker, don't pre-bundle monaco

* fix type check errors

* remove section from readme about committed dist

* keep editor.worker.js postfix

* forgot to save update to import

* license package as apache-2.0

* Added regenerator runtime for worker bundle

* revert changes to painless lab

Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants