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

[Monaco] Refactor the way XJSON grammar checker gets registered #75160

Conversation

jloleysens
Copy link
Contributor

Summary

This contribution makes the following changes:

  • avoid registering multiple model add listeners
  • remove regsiterGrammarChecker from public API
  • fix getWorker handler to register XJSON only for XJSON models

This fixes the workers ability to provide completions for multiple editor instances (two and more) that are using XJSON and it also ensure that we do not double-register a listener (or more times).

How to test

There are not really any instances of the monaco editor being used twice+ simultaneously. I would recommend making the following code changes to test:

  1. Replace the contents of x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/custom.tsx with the code below. This will make the "custom" processor configuration form have 2 editors
  2. Go to Ingest Node Pipelines plugin in the Stack Management section
  3. Edit or create a pipeline
  4. Add a new processor and type in "custom" in the type field and press enter
  5. You should see the two text editors in the flyout
  6. (Bonus) open your browser console, type option+cmd+f and enter XJSON_GRAMMAR_CHECKER in the search box. Select the instance in the language.js file. Place a breakpoint inside of the onDidChangeContent handler (this should be a single call to updateAnnos. Type something in the editor, this should be called once only, even after opening and closing the flyout.
Code
/*
 * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
 * or more contributor license agreements. Licensed under the Elastic License;
 * you may not use this file except in compliance with the Elastic License.
 */

import React, { FunctionComponent } from 'react';
import { i18n } from '@kbn/i18n';

import {
  FieldConfig,
  FIELD_TYPES,
  fieldValidators,
  UseField,
} from '../../../../../../shared_imports';

const { emptyField, isJsonField } = fieldValidators;

import { XJsonEditor } from '../field_components';

const customConfig: FieldConfig = {
  type: FIELD_TYPES.TEXT,
  label: i18n.translate('xpack.ingestPipelines.pipelineEditor.customForm.optionsFieldLabel', {
    defaultMessage: 'Configuration',
  }),
  serializer: (value: string) => {
    try {
      return JSON.parse(value);
    } catch (error) {
      // swallow error and return non-parsed value;
      return value;
    }
  },
  deserializer: (value: any) => {
    if (value === '') {
      return '{\n\n}';
    }
    return JSON.stringify(value, null, 2);
  },
  validations: [
    {
      validator: emptyField(
        i18n.translate(
          'xpack.ingestPipelines.pipelineEditor.customForm.configurationRequiredError',
          {
            defaultMessage: 'Configuration is required.',
          }
        )
      ),
    },
    {
      validator: isJsonField(
        i18n.translate('xpack.ingestPipelines.pipelineEditor.customForm.invalidJsonError', {
          defaultMessage: 'The input is not valid.',
        })
      ),
    },
  ],
};

interface Props {
  defaultOptions?: any;
}

/**
 * This is a catch-all component to support settings for custom processors
 * or existing processors not yet supported by the UI.
 *
 * We store the settings in a field called "customOptions"
 **/
export const Custom: FunctionComponent<Props> = ({ defaultOptions }) => {
  return (
    <>
      <UseField
        path="customOptions"
        component={XJsonEditor}
        config={customConfig}
        defaultValue={defaultOptions}
        componentProps={{
          editorProps: {
            'data-test-subj': 'processorOptionsEditor',
            height: 300,
            'aria-label': i18n.translate(
              'xpack.ingestPipelines.pipelineEditor.customForm.optionsFieldAriaLabel',
              {
                defaultMessage: 'Configuration JSON editor',
              }
            ),
          },
        }}
      />
      <UseField
        path="whatHaveYou"
        component={XJsonEditor}
        config={customConfig}
        defaultValue={defaultOptions}
        componentProps={{
          editorProps: {
            'data-test-subj': 'processorOptionsEditor',
            height: 300,
            'aria-label': i18n.translate(
              'xpack.ingestPipelines.pipelineEditor.customForm.optionsFieldAriaLabel',
              {
                defaultMessage: 'Configuration JSON editor',
              }
            ),
          },
        }}
      />
    </>
  );
};

- avoid registering multiple model add listeners
- remove regsiterGrammarChecker from public API!
- fix getWorker handler to register XJSON only for XJSON models
@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 Feature:Ingest Node Pipelines Ingest node pipelines management v7.9.1 labels Aug 17, 2020
@jloleysens jloleysens requested a review from a team as a code owner August 17, 2020 13:44
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally and made the 2 editor work.

Type something in the editor, this should be called once only

I have put the breakpoint but it didn't get called when changing the editor value. Even after refreshing the browser window. Maybe we can zoom about that?

@@ -3,8 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { XJsonLang } from '@kbn/monaco';
import { XJsonLang, monaco } from '@kbn/monaco';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you added the import but don't use it anywhere

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ingestPipelines 643.8KB -116.0B 643.9KB

History

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

@jloleysens jloleysens merged commit 36f0c75 into elastic:master Aug 18, 2020
@jloleysens jloleysens deleted the kbn-monaco/fix/handle-multiple-editor-instances branch August 18, 2020 13:13
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 18, 2020
…tic#75160)

* Refactor the way XJSON grammar checker gets registered

- avoid registering multiple model add listeners
- remove regsiterGrammarChecker from public API!
- fix getWorker handler to register XJSON only for XJSON models

* remove unused import

* updateAnnos -> updateAnnotations
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 18, 2020
…tic#75160)

* Refactor the way XJSON grammar checker gets registered

- avoid registering multiple model add listeners
- remove regsiterGrammarChecker from public API!
- fix getWorker handler to register XJSON only for XJSON models

* remove unused import

* updateAnnos -> updateAnnotations
# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/xjson_editor.tsx
jloleysens added a commit that referenced this pull request Aug 18, 2020
…) (#75288)

* Refactor the way XJSON grammar checker gets registered

- avoid registering multiple model add listeners
- remove regsiterGrammarChecker from public API!
- fix getWorker handler to register XJSON only for XJSON models

* remove unused import

* updateAnnos -> updateAnnotations
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 18, 2020
* master:
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  [Security Solution][Detections] Adds exception modal tests (elastic#74596)
  [Dashboard] Sample data link does not work (elastic#75262)
  [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905)
  [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166)
  convert processor labels to sentence case (elastic#75278)
  [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160)
  Clarify no documents error message when filtering by is_training (elastic#75227)
  [Lens] Fix crash when two layers xychart  switches to pie (elastic#75267)
  [Observability Homepage] Fix console error because of side effect (elastic#75258)
  [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146)
  [ML] Functional tests - re-activate DFA test suites (elastic#75257)
  GS providers improvements (elastic#75174)
  [Visualize] First version of by-value visualize editor (elastic#72256)
jloleysens added a commit that referenced this pull request Aug 19, 2020
…#75160) (#75291)

* [Monaco] Refactor the way XJSON grammar checker gets registered (#75160)

* Refactor the way XJSON grammar checker gets registered

- avoid registering multiple model add listeners
- remove regsiterGrammarChecker from public API!
- fix getWorker handler to register XJSON only for XJSON models

* remove unused import

* updateAnnos -> updateAnnotations
# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/xjson_editor.tsx

* remove use of registerGrammarChecker in XJsonEditor
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 19, 2020
…emove-header

* saved-objects/version-on-create: (59 commits)
  remove version when loading sample data
  omit version from SO import/export
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  [Security Solution][Detections] Adds exception modal tests (elastic#74596)
  Revert "Revert "added missing core docs""
  Revert "Revert "added version to saved object bulk creation""
  [Dashboard] Sample data link does not work (elastic#75262)
  [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905)
  [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166)
  convert processor labels to sentence case (elastic#75278)
  [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160)
  Clarify no documents error message when filtering by is_training (elastic#75227)
  [Lens] Fix crash when two layers xychart  switches to pie (elastic#75267)
  [Observability Homepage] Fix console error because of side effect (elastic#75258)
  [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146)
  [ML] Functional tests - re-activate DFA test suites (elastic#75257)
  GS providers improvements (elastic#75174)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Ingest Node Pipelines Ingest node pipelines management 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.1 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants