[Console] Move out of quarantined by jloleysens · Pull Request #52270 · elastic/kibana

jloleysens · 2019-12-05T12:33:31Z

Summary

This is a continuation (and completion) of a refactor to the public code in Console started with #43346.

Primary objectives

Move all code out of public/quarantined. This entails reviewing and ensuring that we minimise/demarcate direct usage of brace. We are not planning on switching to Monaco in the immediate future, but this contribution should help a lot with any such future effort.
Finish the CoreEditor interface and implement the new SenseEditor model which uses it.
Leave no test behind! We want to bring along all of our existing test coverage.

Overview of `public/np_ready` directory structure (and architecture):

application should contain all application relevant logic
1. models: contain business logic for our special console language (legacy_core_editor) and knowledge of requests (sense_editor).
  1. legacy_core_editor should contain all wiring up of brace
2. containers: all UI components that interact with context
3. components: all pure/function UI components
4. contexts: contain the mechanism for how we share our flux-like stores ([Console] Refactoring more legacy code and implementing minor fixes #51312)
5. stores: contain the logic for how state can change and the shape of state slices
6. hooks: contain all actions that can affect state changes and don't directly speak to rendering UI
lib
1. Logic which is self-contained and only interacts with our contracts defined in types should live here (e.g., ace_token_provider) surfaces an implementation of TokenProvider without leaking Ace interfaces.
types - folder can be renamed, but named is types to follow conventions in ES UI management apps. This houses only types + interfaces, no real values.

How to review

A lot of changes are just implementing use of the new CoreEditor and SenseEditor interfaces. They contain new logic and will benefit from having their interfaces reviewed!
After moving from row -> lineNumber for our position interfaces there is risk of "off-by-one" regressions. These will probably be very hard to spot, but manual and automated testing has caught a large number of these - but still keep an eye out for them!
I don't expect there is much scope for making a lot of changes to architecture of the application here, it's not that complex, but if there is anything weird please point it out!
Manually test!
- Start Kibana and open Console
- Open History (interact with it)
- Adjust settings
- Send requests, send multi-doc requests

Other goodies

Some fixes were made as part of this PR:

Off by one issue in RowParser.ts's getRowParseMode which had an off-by-one regression (example of fix: if (lineNumber < linesCount + 1)...)
sense_editor.test.js had an issue where tests where not correctly reporting failure 🤦‍♂
Fix for restoring a request by double-clicking: onDoubleClick inside of console_history container

…vider test working. Still need to delete some files

Still working on sense editor's integration test Not running yet.

- remove ace ranges from sense editor spec and fix spec 🤦

elasticmachine · 2019-12-05T12:33:33Z

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

elasticmachine · 2019-12-05T13:40:48Z

💚 Build Succeeded

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

src/legacy/core_plugins/console/public/np_ready/application/components/editor_example.tsx

...ugins/console/public/np_ready/application/containers/editor/legacy/console_editor/editor.tsx

...onsole/public/np_ready/application/containers/editor/legacy/console_editor/editor_output.tsx

...public/np_ready/application/containers/editor/legacy/console_editor/apply_editor_settings.ts

...le/public/np_ready/application/containers/editor/legacy/console_editor/keyboard_shortcuts.ts

...plugins/console/public/np_ready/application/containers/editor/legacy/console_menu_actions.ts

...e/public/np_ready/application/models/legacy_core_editor/__tests__/input_tokenization.test.js

...ore_plugins/console/public/np_ready/application/models/legacy_core_editor/create_readonly.ts

Clean up use of jquery Remove use of `done` inside async tests (input_tokenization.test.js) Capitalize elasticsearch Introduce helper for converting to AceRange inside legacy core editor Update typings Clean up imports Cleaner variable assignment

src/legacy/core_plugins/console/public/np_ready/application/models/sense_editor/curl.ts

pugnascotia · 2019-12-06T14:06:54Z

src/legacy/core_plugins/console/public/np_ready/lib/row_parser.ts

+import { CoreEditor, Token } from '../types';
+import { TokenIterator } from './token_iterator';
+
+export const MODE = {


Feels like a good candidate for conversion to an enum, but not in this PR.

pugnascotia · 2019-12-06T14:10:56Z

src/legacy/core_plugins/console/public/np_ready/types/core_editor.ts

  clearSelection(): void;

+  /**
+   * TODO: Document


Is this TODO supposed to be completed in this PR? The other new functions have documentation.

elasticmachine · 2019-12-06T14:37:14Z

💔 Build Failed

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

…dels/sense_editor/curl.ts Co-Authored-By: Rory Hunter <pugnascotia@users.noreply.github.com>

elasticmachine · 2019-12-06T17:06:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 59ea7d0

cjcenizal

Great work @jloleysens! I tested the scenarios listed in #42029, tried out the different settings, and used the history dropdown. Everything behaved as expected. Code LGTM, just had a few nitpicks and questions but nothing worth blocking on.

The architecture looks like a big improvement. I think we can continue to look for ways to make the the roles of and relationship between SenseEditor and LegacyCoreEditor more apparent. This could be as simple as a name change, or maybe we reconsider whether SenseEditor should be a class and is better broken up into a number of pure functions (just speculating!). Just a casual thought based on some initial confusion I had about these two entities that was partially resolved by reading the source.

cjcenizal · 2019-12-07T00:46:03Z

src/legacy/core_plugins/console/public/np_ready/application/models/sense_editor/curl.ts

+  return false;
+}
+
+export function parseCURL(text: string) {


Wow, I had no idea Console supported curl! This is really impressive but it seems like an undocumented feature which adds maintenance overhead, and I'm not sure it's particularly useful or if people even know about it. What do you think about planning for deprecating this feature in the future, e.g. 8.0?

Check with the Training folks - I have a feeling it was mentioned when I took the ES Engineer 1 training recently.

"Copy as curl" I can see being more useful, but auto-parsing curl is unexpected. Well, to me, anyway.

Yeah, tbh I didn't really know about this feature either 😅 happy to remove in future!

I created #52570 to track my proposal.

src/legacy/core_plugins/console/public/np_ready/application/models/sense_editor/utils.ts

cjcenizal · 2019-12-07T00:54:06Z

src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/autocomplete.ts

-    pos: any,
-    prefix: any,
-    callback: any
+    DO_NOT_USE_SESSION: IEditSession,


I love the way you've named things that are deprecated!

src/legacy/core_plugins/console/public/np_ready/application/models/sense_editor/sense_editor.ts

lib/utils.js -> lib/utils.ts Rename engulfling range (sense_editor)

jloleysens · 2019-12-09T12:39:51Z

@elasticmachine merge upstream

src/legacy/core_plugins/console/public/np_ready/lib/utils/utils.ts

pugnascotia · 2019-12-10T09:20:50Z

src/legacy/core_plugins/console/public/np_ready/lib/utils/utils.ts

-        .replace('^s*\n', '')
-        .replace('\ns*^', '');
+        .replace('^\s*\n', '') // prettier-ignore
+        .replace('\n\s*$', ''); // prettier-ignore


I think Prettier is correct here - these patterns be surrounded in forward-slashes.

const jsonValue = JSON.parse(rawStringifiedValue) .replace(/^\s*\n/, '') .replace(/\n\s*$/, '');

I see what you mean about the difference with trim() - it would remove all whitespace at the start or end of string, whereas these patterns are a little more subtle. Maybe it would be worth a comment, in case someone comes along later and replaces it with trim(). 😀

(Although I still query the usefulness of this vs. a simple trim())

Come to think of it, this replace behaviour was part of legacy and was carried over from a previous alteration. I'm not convinced we want replace behaviour here at all. The value being altered was inside of """ which means string literal in Console. Newlines and whitespace should be preserved. I don't think those replace functions where actually doing anything when the behaviour was considered "correct":

"\n SELECT * FROM \"TABLE\"\n " .replace('^\s*\n', '') .replace('\n\s*$', '') === "\n SELECT * FROM \"TABLE\"\n " // true

Automated and manual testing seem to confirm this. Would you mind taking a look too @pugnascotia ?

elasticmachine · 2019-12-10T11:05:25Z

💚 Build Succeeded

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

History

💚 Build #14249 succeeded a0e39bb
💚 Build #14174 succeeded f3b80b9

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

* WiP, lotta broken things, working through new editor.ts * RowParser -> TS * Utils to TS and regular module * Finished first version of core & sense editor wrappers. Tokenizer provider test working. Still need to delete some files * WiP - moved A LOT of code around and busy fixing sense-editor tests * Fix sense editor test * Clean up mocks * Moved A LOT of code out of quarantined. Still working on sense editor's integration test Not running yet. * WiP still finishing up manual testing * Fix use of Ace Range and fix open documentation * Move out of quarantined! * Remove load remote editor state for now * - fix use of token iterator - remove ace ranges from sense editor spec and fix spec 🤦 * Address getSelectionRange document TODO Clean up use of jquery Remove use of `done` inside async tests (input_tokenization.test.js) Capitalize elasticsearch Introduce helper for converting to AceRange inside legacy core editor Update typings Clean up imports Cleaner variable assignment * Update src/legacy/core_plugins/console/public/np_ready/application/models/sense_editor/curl.ts Co-Authored-By: Rory Hunter <pugnascotia@users.noreply.github.com> * Remove commented-out code lib/utils.js -> lib/utils.ts Rename engulfling range (sense_editor) * Rename format request function * utils.ts default export usage cleanup * Update replace regex and add another utils test * Remove legacy replace behaviour * Fix typo in comment

jloleysens added 13 commits December 5, 2019 11:01

WiP, lotta broken things, working through new editor.ts

f53964c

RowParser -> TS

ba78edd

Utils to TS and regular module

d92621c

Finished first version of core & sense editor wrappers. Tokenizer pro…

05bb02b

…vider test working. Still need to delete some files

WiP - moved A LOT of code around and busy fixing sense-editor tests

18d0093

Fix sense editor test

0f2ef09

Clean up mocks

458cd57

Moved A LOT of code out of quarantined.

78bbd57

Still working on sense editor's integration test Not running yet.

WiP still finishing up manual testing

47f5191

Fix use of Ace Range and fix open documentation

f733e8f

Move out of quarantined!

6feb90c

Remove load remote editor state for now

3d0259f

- fix use of token iterator

dd2f3e1

- remove ace ranges from sense editor spec and fix spec 🤦

jloleysens added Feature:Console Dev Tools Console Feature v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Dec 5, 2019

jloleysens requested review from cjcenizal and pugnascotia December 5, 2019 12:33