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

Move painless lang support to @kbn/monaco #81010

Merged

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Oct 19, 2020

This PR includes some refactoring of the @kbn/monaco package. I started this work in #80577, but decided to break it up as a separate PR as it affects the XJSON support.

  • Created top-level folders for painless and esql languages, parallel to xjson. The painless language will eventually include autocomplete and error handling support (to be implemented in Support for painless language autocomplete within monaco #80577). I imagine we might at some point want to provide the same for esql too.
  • Created a global.ts file that registers the languages and creates the web workers. There will eventually be a web worker for painless.
  • Removed the languageService in painless_lab. AFAICT this is no longer necessary.

How to test

Verify there are no regressions in the XJSON support and syntax highlighting in painless lab. I'm fairly confident in the painless lab changes, but not so much for XJSON 😄 . I used the sample in #67485 to test, but there may be more use cases I should verify (in particular around esql support).

@alisonelizabeth alisonelizabeth 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 Feature:Painless Lab Dev tool for learning Painless v7.11.0 labels Oct 19, 2020
@@ -41,8 +34,7 @@ interface Language extends monaco.languages.IMonarchLanguage {
}

export const lexerRules = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see, there are a few minor differences between the painless lexer rules in xjson (original code) and the lexer rules in painless lab (original code). I don't see why they would/should be different, but if there is a reason, please let me know.

@@ -189,6 +180,3 @@ export const lexerRules = {
],
},
} as Language;

monaco.languages.register({ id: ID });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed the xjson and painless languages were getting registered twice - once here (and in lexer_rules/xjson, respectively), and again in the registerLexerRules function. I assume this was a mistake, but wanted to call out here if not.

@alisonelizabeth alisonelizabeth marked this pull request as ready for review October 19, 2020 17:03
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner October 19, 2020 17:03
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @alisonelizabeth ! I tested locally and everything seems to be working as expected!

I noticed this warning from Monaco when opening a page, like Painless lab, with a painless editor in it:

Screenshot 2020-10-20 at 11 31 43

I think we need to re-introduce the behaviour of language_service.ts that did the following:

import workerSrc from 'raw-loader!monaco-editor/min/vs/base/worker/workerMain.js';
...
      (window as any).MonacoEnvironment = {	
        getWorker: () => {	
          const blob = new Blob([workerSrc], { type: 'application/javascript' });	
          return new Worker(window.URL.createObjectURL(blob));	
        },
...

In the global.ts file you created we should just add an if check for painless to suppress this warning. We also need to do this for Elasticsearch SQL esql.

One other thing that I want to get your thoughts on, non-blocking: I think renaming global.ts -> register_globals.ts and changing monaco.ts -> monaco_imports.ts might better communicate the intention behind these files. That way if I see import './reigster_globals' in the code it seems even clearer regarding the intention behind the file.

Overall these changes are looking great!

@alisonelizabeth
Copy link
Contributor Author

Thanks @jloleysens for the review!

One other thing that I want to get your thoughts on, non-blocking: I think renaming global.ts -> register_globals.ts and changing monaco.ts -> monaco_imports.ts might better communicate the intention behind these files. That way if I see import './reigster_globals' in the code it seems even clearer regarding the intention behind the file.

👍 Good call. I went ahead and made this change. I think this is more explicit as well.

I noticed this warning from Monaco when opening a page, like Painless lab, with a painless editor in it:

Good catch! For some reason, I don't recall seeing the warnings when I was doing initial testing, which is why I removed it. This should be fixed now via ac180cc.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
painlessLab 40 35 -5

@kbn/ui-shared-deps asset size

id before after diff
kbn-ui-shared-deps.@elastic.js 2.4MB 2.4MB +8.0B
kbn-ui-shared-deps.js 4.7MB 4.8MB +90.5KB
total +90.5KB

async chunks size

id before after diff
painlessLab 38.7KB 38.8KB +104.0B

distributable file count

id before after diff
default 48052 48060 +8
oss 28571 28579 +8

page load bundle size

id before after diff
painlessLab 160.9KB 27.0KB -133.9KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Painless Lab Dev tool for learning Painless 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.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants