Add Syntax Highlighting for SQL Input #373

mknowlton89 · 2022-06-07T21:01:45Z

Empty State

Edit State

This PR handles adding support for the react-ace package that enables users to write sql with syntax highlighting.

Currently, the implementation only supports SQL, but we can expand that to other languages if needed. Additionally, we can choose between an array of different themes. Currently, the component is consuming the sqlserver theme, but that can be easily changed.

This update will be reflected in the following places within the app:

SegmentForm
Metrics
Dimensions
Data Sources -> Edit Query Settings Modal

To Do:

Update the component to take in a value (if one exists) so when editing, the existing SQL query is shown.
Ensure that the changes look good on mobile. (Although, the chances of someone writing SQL on their phone is probably low, lol)
Add the new component to the areas listed above.
Update the placeholder text in SegmentForm, and DimensionForm to use a template literal so the spacing/alignment is accurate rather than it being a single line.
Determine what tests, if any, I need to build for this.
- Spoke to Jeremy, we're going to add tests to the frontend package, but not yet, so no tests for this PR.
Determine if/how to add syntax error highlighting in the component.
- So, it is possible to add error highlighting, but I've not been able to get it to work in our environment. There is a prop called onValidate that gets what react-ace calls annotations. Annotations is an array of errors, that is passed in to the annotations prop. However, looking at the source code for react-ace, onValidate only runs inside of componentDidMount, so its called correctly in our env with the component mounts, but then never gets called again whenever the input changes.
  I've looked everywhere for a good example of using react-ace with a nextjs app, but haven't been able to. Ace supports live syntax checking using a WebWorker. Perhaps this is where our issue lies - I'll do a little research on WebWorkers to see if that's where our blocker is.
Determine if we want to open the component up to support more languages.
- Spoke to Jeremy and we do want to support: Python, Javascript, JSON, SQL, & YAML. I've updated this component to support these languages.
- For this PR, since I'm already touching the EditDataSourceSettings component, which has both a SQL textarea and a Python textarea, I'm going to update both, but all other non-sql textareas I will leave for another PR.
Determine if there are any other spots in the app where we might need to consume this new component.
- I don't think there are any other textareas for sql code, but as noted, there are other codeblocks. I think the scope of this PR is to just add update the textareas where users can write sql. Will double check with Jeremy.

General Questions:
In the Data Sources -> Edit Query Settings Modal there is a textarea where users can write Python, I can update the SqlTextArea to take in a prop for the language/mode/theme to support Python as well as SQL. Should I do this here or in a separate ticket?

~~Alternatively, we could break the PythonTextArea into it's own component, but there's so much shared between the two it's probably not needed.~~

…t I haven't been able to successfully wire up the new component to work with the useForm hook. Once I get that working, I'll take a step back and evaluate if there are any big architectural changes I need to make before using the new SqlTextEditor component in other areas of the app.

vercel · 2022-06-07T21:01:48Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
docs	⬜️ Ignored (Inspect)		Jun 10, 2022 at 7:24PM (UTC)

… take a look at the overall architecture and identify any changes to be made before replacing other text areas with the new SqlTextArea component.

…ad/understand. I created a ternary for the <Field> component in <SegmentForm> and broke out the <Field> component based on if sql was true, instead of trying to do everything in-line with a single <Field> component. The nested ternaries were getting out of hand.

…st so I had record of it if I need to go back to it.

…istingValue'. Also updated the DimensionForm component to consume the new SqlTextArea component.

… placeholder text to use template literals with formatting.

…ettings component.

…omponent.

packages/front-end/components/Segments/SegmentForm.tsx

mknowlton89 · 2022-06-08T18:02:31Z

I would love some help testing this to make sure that the value ultimately being passed in to the form.handleSubmit function is the correct shape. I tested it with and without my changes, and they appear to be the same, but I'd just love some confirmation.

The shape of the data being passed to the form.handleSubmit function is as follows.

datasource: "ds_exl5jbhel44g751t"
name: "Test"
sql: "SELECT\n   user_id, date\nFROM\n  growthbook\n\n"
userIdType: "user_id"

Specifically want to confirm that the \n for new lines won't interfere with the functionality and that we don't need to sanitize this at all.

packages/front-end/components/Settings/EditDataSourceSettingsForm.tsx

…ort for javascript, python, yaml, and json. Also made the height adjustable via a prop for future customization.

… updated the standard height of the CodeTextArea component to be 140px instead of 150 as I found other areas in the app referencing 140. Also removed duplicate codeTextAreaHeight props that weren't doing anything since we have a fallback height set.

…he CodeTextArea renders the Field, based on Jeremy's feedback. The Field component was getting really large and complicated.

…found one other sql text area that needed to be refactored to the new component.

jdorn · 2022-06-10T13:56:03Z

packages/front-end/components/Dimensions/DimensionForm.tsx

+  {
+    ssr: false,
+  }
+);


Dynamic imports make sense for performance since Ace Editor is so big, but I don't like having to copy/paste this block of code everywhere. Can we move the dynamic import into the CodeTextArea file instead? That way we can just do normal imports throughout the app.

So, I'm trying to simplify things by moving the dynamic import into the CodeTextEditor so we only have to do it once. But I originally used the dynamic import to resolve an issue other than performance.

Without the dynamic import, upon page refresh, I was getting a window not defined error that I could only resolve by dynamically importing it.

Now, when I move the dynamic import in to the CodeTextEditor I'm getting an error of ace is not defined so I'm working through that now.

Alrighty - just got it working, not sure if I love the implementation, so let me know what you think.

packages/front-end/components/Forms/CodeTextArea.tsx

…red out how to move the dynamic import into the CodeTextArea.

packages/front-end/components/Forms/CodeTextArea.tsx

jdorn · 2022-06-10T19:07:26Z

packages/front-end/components/Settings/EditDataSourceSettingsForm.tsx

-                      maxRows={20}
+                      value={form.watch(`settings.notebookRunQuery`)}
+                      setValue={(sql) =>
+                        form.setValue(`settings.notebookRunQuery`, sql)


This variable name should not be sql since it's dealing with python code instead

DOH! Embarrassing. I've updated this. I also realized that line 458 (I'm pretty sure) is not necessary now that I've added the value/setValue props. So I've removed.

… updated EditDataSourceSettings setValue prop.

mknowlton89 self-assigned this Jun 7, 2022

mknowlton89 linked an issue Jun 7, 2022 that may be closed by this pull request

Syntax Highlighting SQL Input #372

Closed

vercel bot deployed to Preview June 7, 2022 21:03 View deployment

Michael Knowlton added 5 commits June 8, 2022 08:59

Got the new component wired up to the form correctly. Next step is to…

30dc25f

… take a look at the overall architecture and identify any changes to be made before replacing other text areas with the new SqlTextArea component.

Cleaning up the old implementation. I commited it in the last push ju…

09d719f

…st so I had record of it if I need to go back to it.

Fixed a type issue by renaming the 'value' prop in SqlTextArea to 'ex…

64b8146

…istingValue'. Also updated the DimensionForm component to consume the new SqlTextArea component.

Added the SqlTextArea component to the MetricForm, and refactored the…

2fddfc8

… placeholder text to use template literals with formatting.

mknowlton89 changed the title ~~First pass at adding sql syntax highlighting. It's somewhat close, bu…~~ Add Syntax Highlighting for SQL Input Jun 8, 2022

Michael Knowlton added 3 commits June 8, 2022 13:02

Added the SqlTextArea component to be consumed by the EditDataSourceS…

f630805

…ettings component.

Cleaning up the prop name so it's the proper case.

a9af614

Forgot to remove a prop that is no longer being used from the Field c…

67466e4

…omponent.

mknowlton89 commented Jun 8, 2022

View reviewed changes

packages/front-end/components/Segments/SegmentForm.tsx Show resolved Hide resolved

Cleaned up some language and casing.

66e06c7

mknowlton89 commented Jun 8, 2022

View reviewed changes

packages/front-end/components/Settings/EditDataSourceSettingsForm.tsx Outdated Show resolved Hide resolved

Michael Knowlton added 2 commits June 9, 2022 10:00

Renamed the component from sqlTextArea to codeTextArea and added supp…

32d0284

…ort for javascript, python, yaml, and json. Also made the height adjustable via a prop for future customization.

mknowlton89 marked this pull request as ready for review June 9, 2022 20:32

mknowlton89 requested review from jdorn, Auz and mirabali June 9, 2022 20:32

Michael Knowlton added 3 commits June 10, 2022 08:51

Refactored so instead of the Field component rendering CodeTextArea t…

c516096

…he CodeTextArea renders the Field, based on Jeremy's feedback. The Field component was getting really large and complicated.

Added back python placeholder text that I accidentally omitted. Also …

0f46ad1

…found one other sql text area that needed to be refactored to the new component.

Updated the label so it matched what was previously in production.

a7cc11e

jdorn reviewed Jun 10, 2022

View reviewed changes

Michael Knowlton added 2 commits June 10, 2022 11:07

Addressed the first level of feedback from Jeremy. Still haven't figu…

860c1d8

…red out how to move the dynamic import into the CodeTextArea.

Updated the CodeTextArea so we only use the dynamic import once.

20cf4cc

mknowlton89 requested a review from jdorn June 10, 2022 15:36

jdorn reviewed Jun 10, 2022

View reviewed changes

Adding placeholder back in as a destructured prop to CodeTextArea and…

f4bd0bb

… updated EditDataSourceSettings setValue prop.

mknowlton89 requested a review from jdorn June 10, 2022 19:26

jdorn merged commit 69eef29 into main Jun 13, 2022

jdorn deleted the michael/add-acejs-sql-syntax-editor branch June 13, 2022 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Syntax Highlighting for SQL Input #373

Add Syntax Highlighting for SQL Input #373

mknowlton89 commented Jun 7, 2022 •

edited

Loading

vercel bot commented Jun 7, 2022 •

edited

Loading

mknowlton89 commented Jun 8, 2022 •

edited

Loading

jdorn Jun 10, 2022

mknowlton89 Jun 10, 2022

mknowlton89 Jun 10, 2022

jdorn Jun 10, 2022

mknowlton89 Jun 10, 2022 •

edited

Loading

Add Syntax Highlighting for SQL Input #373

Add Syntax Highlighting for SQL Input #373

Conversation

mknowlton89 commented Jun 7, 2022 • edited Loading

vercel bot commented Jun 7, 2022 • edited Loading

mknowlton89 commented Jun 8, 2022 • edited Loading

jdorn Jun 10, 2022

Choose a reason for hiding this comment

mknowlton89 Jun 10, 2022

Choose a reason for hiding this comment

mknowlton89 Jun 10, 2022

Choose a reason for hiding this comment

jdorn Jun 10, 2022

Choose a reason for hiding this comment

mknowlton89 Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

mknowlton89 commented Jun 7, 2022 •

edited

Loading

vercel bot commented Jun 7, 2022 •

edited

Loading

mknowlton89 commented Jun 8, 2022 •

edited

Loading

mknowlton89 Jun 10, 2022 •

edited

Loading