Validate Painless Lab index field #60841

cjcenizal · 2020-03-21T00:46:52Z

This required significant refactoring of the store's shape. To make this refactor easier, I also tried to simplify the way types and constants are organized. An additional improvement I made was to clarify the labeling of various form fields.

elasticmachine · 2020-03-21T00:48:55Z

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

cjcenizal · 2020-03-21T00:49:44Z

x-pack/plugins/painless_lab/public/application/context.tsx


-  const updateState = (getNextState: (s: Store) => Partial<Store>): void => {


@jloleysens Was there a particular reason to accept a callback here instead of the state change?

The primary reason was to provide callers of updateState with a reference to the previous state so they can make decisions about it if needed, e.g., updateState(s => s.something ? ... : ...) and so that it is a bit more convenient to work with as the store grows in complexity (otherwise we have to fetch the store from const { store } = useAppContext() and use it when we update something and the update it again in future if the shape changes. So it is primarily a mechanism of optimisation.

This followed the useState pattern, so I am happy to have it accept both a callback and an object 👍

OK, thanks for explaining! I follow your reasoning until this part:

otherwise we have to fetch the store from const { store } = useAppContext() and use it when we update something and then update it again in the future if the shape changes.

This is where I get lost. Is there an example of this in the codebase? Philosophically I would suggest waiting until this becomes a problem before introducing a solution for it.

cjcenizal · 2020-03-21T00:51:26Z

x-pack/plugins/painless_lab/public/application/context.tsx

  services: {
    http: HttpSetup;
  };
  links: Links;
 }

+const initialPayload = {


@jloleysens What do you think of bringing this into this file? It's not consumed elsewhere so I think it creates noise to extract it. If we want to break this file's code up into multiple files, I'd prefer to convert it into module (a directory with an index file) to make it clear what's intended for consumption elsewhere.

Sure, I am happy with this change. I had the store.ts file separate so that if we choose to use a reducer in future we can just add the reducer to the store file.

I do think if we start to expand more on validators it would be nice to have a separate file for these. Ideally I like to keep the context file as minimal as possible so that it only has the concern setting up the context. Where, at the moment, it houses store setup and validation logic - perhaps worth creating the folder structure you were speaking about. That way future engineers know where to put things?

OK, I'll create a folder now. In terms of validation, I would expect us to try using the form hook lib if we want to add more rules in the future.

jloleysens

@cjcenizal thanks for adding some validation to the store! I think it's a pattern we can build upon in future. I did not test this locally.

I left one comment regarding state init that I think needs be addressed before merging though!

(Approving to not block because the change is minor, incl. the "yay" param)

jloleysens · 2020-03-23T09:34:57Z

x-pack/plugins/painless_lab/public/application/context.tsx

  services: {
    http: HttpSetup;
  };
  links: Links;
 }

+const initialPayload = {


Sure, I am happy with this change. I had the store.ts file separate so that if we choose to use a reducer in future we can just add the reducer to the store file.

jloleysens · 2020-03-23T09:35:14Z

x-pack/plugins/painless_lab/public/application/context.tsx

+  context: painlessContextOptions[0].value,
+  code: exampleScript,
+  parameters: `{
+  "string-parameter": "yay",


Should we clean these up?

Or are they intended as the parameter defaults? "yay" seems a bit too informal to me 😄

Haha yes I intended them to be defaults. I'll try to find something appropriately solemn. :)

jloleysens · 2020-03-23T09:39:03Z

x-pack/plugins/painless_lab/public/application/context.tsx

  services: {
    http: HttpSetup;
  };
  links: Links;
 }

+const initialPayload = {


I do think if we start to expand more on validators it would be nice to have a separate file for these. Ideally I like to keep the context file as minimal as possible so that it only has the concern setting up the context. Where, at the moment, it houses store setup and validation logic - perhaps worth creating the folder structure you were speaking about. That way future engineers know where to put things?

jloleysens · 2020-03-23T09:45:55Z

x-pack/plugins/painless_lab/public/application/context.tsx

-    ...initialState,
+  const PAINLESS_LAB_KEY = 'painlessLabState';
+
+  const defaultPayload = {


This should be inside of a getter so that we have this pattern: useState(() => ...). In this implementation we are calling JSON.parse and localStorage.getItem for every state update. This post explains the lazy init idea: https://dmitripavlutin.com/react-usestate-hook-guide/#3-lazy-initialization-of-state

Thanks for sharing that link! I was completely overlooking that performance hit and the link opened my eyes. Will fix.

jloleysens · 2020-03-23T09:50:10Z

x-pack/plugins/painless_lab/public/application/context.tsx


-  const updateState = (getNextState: (s: Store) => Partial<Store>): void => {


The primary reason was to provide callers of updateState with a reference to the previous state so they can make decisions about it if needed, e.g., updateState(s => s.something ? ... : ...) and so that it is a bit more convenient to work with as the store grows in complexity (otherwise we have to fetch the store from const { store } = useAppContext() and use it when we update something and the update it again in future if the shape changes. So it is primarily a mechanism of optimisation.

This followed the useState pattern, so I am happy to have it accept both a callback and an object 👍

…meters to provide an example to users. - Move PAINLESS_LAB_KEY constant into context file, since that's the only place it's being used.

- Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used.

- This required a refactor to the shape of our store.

cjcenizal · 2020-03-23T20:21:56Z

Thanks for the review @jloleysens! I've addressed all of your feedback except for your suggestion to allow updatePayload to accept both a payload object and a callback. I'd be happy to implement this in a separate PR if you feel strongly about it.

kibanamachine · 2020-03-23T21:50:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 87a7f58

History

💚 Build #35138 succeeded 5aadb7764d1abe72e5363ffb0a184a96a63080b1

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

* Create Painless Playground app (#54578) * Replace heart script with smiley face script. (#57755) * Rename Painless Playground -> Painless Lab. (#57545) * Fix i18n namespace. * Improve smiley face proportions. - Add def keyword to Painless spec. - Temporarily fix broken highlighting. - Add small padding to main controls. * [Painless Lab] Minor Fixes (#58135) * Code restructure, improve types, add plugin id, introduced hook Moved the code execution hook to a custom hook outside of main, also chaining off promise to avoid lower level handling of sequencing. * Re-instated formatting code To improve DX the execution error response from the painless API was massaged to a more reader friendly state, only giving non-repeating information. Currently it is hard to determine the line and character information from the painless endpoint. If the user wishes to see this raw information it will be available in the API response flyout. * Remove leading new line in default script * Remove registration of feature flag * Fix types * Restore previous auto-submit request behaviour * Remove use of null and remove old comment Stick with "undefined" as the designation for something not existing. * [Painless Lab] NP migration (#59794) * Fix sample document editor. * [Painless Lab] Fix float -> integer coercion bug (#60201) * Clarify data and persistence flow. Fix floating point precision bug. * Send a string to API and ES client instead of an object. * Rename helpers lib to format. Add tests for formatRequestPayload. * Add query parameter to score context (#60414) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * Fix borked i18n * Fix i18n * Another i18n issue * [Painless] Minor state update model refactor (#60532) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * WiP on state refactor * Some cleanup after manual testing * Fix types and i18n * Fix i18n in context_tab * i18n * [Painless] Language Service (#60612) * Added language service * Use the correct monaco instance and add wordwise operations * Remove plugin context initializer for now * [Painless] Replace hard-coded links (#60603) * Replace hard-coded links Also remove all props from Main component * Pass the new links object to the request flyout too * Link directly to painless execute API's contexts * Remove responsive stacking from tabs with icons in them. * Resize Painless Lab bottom bar to accommodate nav drawer width (#60833) * Validate Painless Lab index field (#60841) * Make JSON format of parameters field more prominent. Set default parameters to provide an example to users. * Set default document to provide an example to users. * Simplify context's updateState interface. * Refactor store and context file organization. - Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used. * Add validation for index input. * Create context directory. * Fix bottom bar z-index. * Position flyout help link so it's bottom-aligned with the title and farther from the close button. Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]>

* Create Painless Playground app (#54578) * Replace heart script with smiley face script. (#57755) * Rename Painless Playground -> Painless Lab. (#57545) * Fix i18n namespace. * Improve smiley face proportions. - Add def keyword to Painless spec. - Temporarily fix broken highlighting. - Add small padding to main controls. * [Painless Lab] Minor Fixes (#58135) * Code restructure, improve types, add plugin id, introduced hook Moved the code execution hook to a custom hook outside of main, also chaining off promise to avoid lower level handling of sequencing. * Re-instated formatting code To improve DX the execution error response from the painless API was massaged to a more reader friendly state, only giving non-repeating information. Currently it is hard to determine the line and character information from the painless endpoint. If the user wishes to see this raw information it will be available in the API response flyout. * Remove leading new line in default script * Remove registration of feature flag * Fix types * Restore previous auto-submit request behaviour * Remove use of null and remove old comment Stick with "undefined" as the designation for something not existing. * [Painless Lab] NP migration (#59794) * Fix sample document editor. * [Painless Lab] Fix float -> integer coercion bug (#60201) * Clarify data and persistence flow. Fix floating point precision bug. * Send a string to API and ES client instead of an object. * Rename helpers lib to format. Add tests for formatRequestPayload. * Add query parameter to score context (#60414) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * Fix borked i18n * Fix i18n * Another i18n issue * [Painless] Minor state update model refactor (#60532) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * WiP on state refactor * Some cleanup after manual testing * Fix types and i18n * Fix i18n in context_tab * i18n * [Painless] Language Service (#60612) * Added language service * Use the correct monaco instance and add wordwise operations * Remove plugin context initializer for now * [Painless] Replace hard-coded links (#60603) * Replace hard-coded links Also remove all props from Main component * Pass the new links object to the request flyout too * Link directly to painless execute API's contexts * Remove responsive stacking from tabs with icons in them. * Resize Painless Lab bottom bar to accommodate nav drawer width (#60833) * Validate Painless Lab index field (#60841) * Make JSON format of parameters field more prominent. Set default parameters to provide an example to users. * Set default document to provide an example to users. * Simplify context's updateState interface. * Refactor store and context file organization. - Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used. * Add validation for index input. * Create context directory. * Fix bottom bar z-index. * Position flyout help link so it's bottom-aligned with the title and farther from the close button. Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]> Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]>

cjcenizal force-pushed the painless/validate-index branch from 0a84482 to 5aadb77 Compare March 21, 2020 00:48

cjcenizal requested a review from jloleysens March 21, 2020 00:48

cjcenizal added painless 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 labels Mar 21, 2020

cjcenizal added the Feature:Dev Tools label Mar 21, 2020

cjcenizal commented Mar 21, 2020

View reviewed changes

jloleysens approved these changes Mar 23, 2020

View reviewed changes

cjcenizal added 9 commits March 23, 2020 12:03

Make JSON format of parameters field more prominent. Set default para…

e9192a1

…meters to provide an example to users. - Move PAINLESS_LAB_KEY constant into context file, since that's the only place it's being used.

Set default document to provide an example to users.

c673c72

Simplify context's updateState interface.

6d48b03

Refactor store and context file organization.

7b4bbe5

- Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used.

Add validation for index input.

2647918

- This required a refactor to the shape of our store.

Create context directory.

be255b8

Use useState callback.

6daffe2

Use example values that aren't so goofy.

69631a4

Fix TS bugs.

87a7f58

cjcenizal force-pushed the painless/validate-index branch from 5aadb77 to 87a7f58 Compare March 23, 2020 20:05

cjcenizal merged commit d0d3036 into elastic:app/painless Mar 23, 2020

cjcenizal deleted the painless/validate-index branch March 23, 2020 20:24

cjcenizal mentioned this pull request Mar 23, 2020

[Painless Lab] Index field should be required in UI #60020

Closed

cjcenizal added the Feature:Painless Lab Dev tool for learning Painless label Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Painless Lab index field #60841

Validate Painless Lab index field #60841

cjcenizal commented Mar 21, 2020 •

edited

Loading

elasticmachine commented Mar 21, 2020

cjcenizal Mar 21, 2020

jloleysens Mar 23, 2020 •

edited

Loading

cjcenizal Mar 23, 2020

cjcenizal Mar 21, 2020 •

edited

Loading

jloleysens Mar 23, 2020

jloleysens Mar 23, 2020

cjcenizal Mar 23, 2020

jloleysens left a comment •

edited

Loading

jloleysens Mar 23, 2020

jloleysens Mar 23, 2020

jloleysens Mar 23, 2020

cjcenizal Mar 23, 2020

jloleysens Mar 23, 2020

jloleysens Mar 23, 2020 •

edited

Loading

cjcenizal Mar 23, 2020

jloleysens Mar 23, 2020 •

edited

Loading

cjcenizal commented Mar 23, 2020

kibanamachine commented Mar 23, 2020


		const updateState = (getNextState: (s: Store) => Partial<Store>): void => {

Validate Painless Lab index field #60841

Validate Painless Lab index field #60841

Conversation

cjcenizal commented Mar 21, 2020 • edited Loading

elasticmachine commented Mar 21, 2020

Choose a reason for hiding this comment

jloleysens Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Mar 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Mar 23, 2020

kibanamachine commented Mar 23, 2020

💚 Build Succeeded

History

cjcenizal commented Mar 21, 2020 •

edited

Loading

jloleysens Mar 23, 2020 •

edited

Loading

cjcenizal Mar 21, 2020 •

edited

Loading

jloleysens left a comment •

edited

Loading

jloleysens Mar 23, 2020 •

edited

Loading

jloleysens Mar 23, 2020 •

edited

Loading