-
-
Notifications
You must be signed in to change notification settings - Fork 657
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
Upgrade to CodeMirror 6, add SQL autocomplete #1893
Conversation
@@ -1,38 +1,43 @@ | |||
<script> | |||
window.onload = () => { | |||
window.onload = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a separate commit, but switching this to DOMContentLoaded should prevent the flash-of-uncodemirrored-object that can happen between the first render and the load event firing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #1894 to track
Should also minify the bundled output |
extraKeys is done - Shift+Enter is added in the helper function, and it appears that the Tab behavior now defaults to what the |
https://github.com/Sphinxxxx/cm-resize isn't compatible with 6. There's a suggestion to try using CSS resize in https://discuss.codemirror.net/t/resizing-codemirror-6/3265/2 |
datasette/static/cm-editor.js
Outdated
], | ||
}); | ||
|
||
// Idea taken from https://discuss.codemirror.net/t/resizing-codemirror-6/3265. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me if this ResizeObserver and requestMeasure is needed. It's suggested in the thread but from some light manual testing the behavior seems the same with or without
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For manual testing, any changes to this file require the build as outlined in the docs:
node_modules/.bin/rollup datasette/static/cm-editor.js -f iife -n cm -o datasette/static/cm-editor.bundle.js \
-p @rollup/plugin-node-resolve -p @rollup/plugin-terser
I experimented with autocompleting the actual schema in bgrins@8431c98, but it would need some work (current problems with it listed in the commit message there) |
I just deployed a demo instance like this (using the commit hash from this PR): datasette publish vercel fixtures.db \
--branch 544f7025900b78f63c34b9985522271ba5fd9c0f \
--project datasette-pr-1893 \
--scope datasette \
--about 'PR 1893' \
--about_url https://github.com/simonw/datasette/pull/1893 Here's the result: https://datasette-pr-1893.vercel.app/fixtures |
Resizing works great for me - and the page automatically sizes the editor to fit an existing query, e.g. on https://datasette-pr-1893.vercel.app/fixtures?sql=select+id%2C+content%2C+content2%0D%0A++from+primary_key_multiple_columns_explicit_label%0D%0A++order+by+id%0D%0A++limit+101 |
datasette/templates/_codemirror.html
Outdated
<link rel="stylesheet" href="{{ base_url }}-/static/codemirror-5.57.0.min.css" /> | ||
<script src="{{ base_url }}-/static/codemirror-5.57.0-sql.min.js"></script> | ||
<script src="{{ base_url }}-/static/cm-resize-1.0.1.min.js"></script> | ||
<script src="{{ base_url }}-/static/cm-editor.bundle.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having the CodeMirror version in the filename, partly to ensure cache busting but mainly because that way I can see at a glance which version was most recently vendored into Datasette.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
datasette/static/cm-editor.js
Outdated
}, | ||
]), | ||
sql({ | ||
dialect: SQLite, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we're seeing all of those weird non-SQLite keywords in autocomplete while using the SQLite dialect here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like those come from these lists here: https://github.com/codemirror/lang-sql/blob/ebf115fffdbe07f91465ccbd82868c587f8182bc/src/tokens.ts#L147-L148
I'm not sure why the SQLite dialect isn't over-riding them. Maybe a missing feature in codemirror/lang-sql
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... weirdly I can't find the implementation of the SQLite dialect in that repo at all? https://github.com/search?q=repo%3Acodemirror/lang-sql%20sqlite&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, GitHub code search let me down there. I found it in the code here: https://github.com/codemirror/lang-sql/blob/ebf115fffdbe07f91465ccbd82868c587f8182bc/src/sql.ts#L231-L239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's that full block of code:
/// [SQLite](https://sqlite.org/) dialect.
export const SQLite = SQLDialect.define({
keywords: SQLKeywords + "abort analyze attach autoincrement conflict database detach exclusive fail glob ignore index indexed instead isnull notnull offset plan pragma query raise regexp reindex rename replace temp vacuum virtual",
types: SQLTypes + "bool blob long longblob longtext medium mediumblob mediumint mediumtext tinyblob tinyint tinytext text bigint int2 int8 unsigned signed real",
builtin: "auth backup bail changes clone databases dbinfo dump echo eqp explain fullschema headers help import imposter indexes iotrace lint load log mode nullvalue once print prompt quit restore save scanstats separator shell show stats system tables testcase timeout timer trace vfsinfo vfslist vfsname width",
operatorChars: "*+-%<>!=&|/~",
identifierQuotes: "`\"",
specialVar: "@:?$"
})
I think the problem here is that it adds SQLKeywords
to that list.
We'd probably be best off defining our own simpler dialect here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at that code I think it'll be pretty easy to define our own dialect, given you have specific keywords/types/builtins in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, here's the SQLKeywords and SQLTypes. Some of those will clearly be applicable here but also sounds like many aren't:
Would be nice if we could pull a similar list directly from SQLite grammar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed with
--- a/datasette/static/cm-editor-6.0.1.js
+++ b/datasette/static/cm-editor-6.0.1.js
@@ -1,7 +1,18 @@
import { EditorView, basicSetup } from "codemirror";
import { keymap } from "@codemirror/view";
-import { sql, SQLite } from "@codemirror/lang-sql";
+import { sql, SQLDialect } from "@codemirror/lang-sql";
+const SQLite = SQLDialect.define({
+ keywords:
+ "abort analyze attach autoincrement conflict database detach exclusive fail glob ignore index indexed instead isnull notnull offset plan pragma query raise regexp reindex rename replace temp vacuum virtual",
+ types:
+ "bool blob long longblob longtext medium mediumblob mediumint mediumtext tinyblob tinyint tinytext text bigint int2 int8 unsigned signed real",
+ builtin:
+ "auth backup bail changes clone databases dbinfo dump echo eqp explain fullschema headers help import imposter indexes iotrace lint load log mode nullvalue once print prompt quit restore save scanstats separator shell show stats system tables testcase timeout timer trace vfsinfo vfslist vfsname width",
+ operatorChars: "*+-%<>!=&|/~",
+ identifierQuotes: '`"',
+ specialVar: "@:?$",
+});
that it updates the dialect as we'd expect (not highlighting or autocompleting SQLKeywords that aren't prepended anymore, like select
) - so looks like it's only a matter of deciding on the exact lists to get that fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify this a lot by skipping everything that's not used in regular SELECT
queries - anything to do with alter table, insert, update and so on.
Datasette may well support SQL queries that do more than just select data in the future (it already does via the https://datasette.io/plugins/datasette-write plugin) but I'm happy for us to revisit the grammar at that point - we can even serve a different grammar to the version of CodeMirror that's attached to a SQL textarea which can perform more than just SELECT queries.
If you can get a version of this working with table and column autocompletion just using a static JavaScript object in the source code with the right tables and columns, I'm happy to take on the work of turning that static object into something that Datasette includes in the page itself with all of the correct values. |
Oops, the tests are failing because of a test failure I introduced here: |
If you rebase from |
Was just reviewing the SQL options and there's an upperCaseKeywords if we'd rather have SELECT vs select. Datasette seems to prefer lowercase so probably best to keep it as-is |
Yeah I haven't written this down anywhere but Datasette definitely has an undocumented preference for lower-case SQL. |
This version "sort of" works when on the main database page where the template passes the relevant data bgrins@8431c98 by doing this and passing that into the
But there are a number of papercuts with it - it's not escaping table names with spaces (likely be fixable from the data being passed into the view) but mainly it doesn't seem to autocomplete columns. I think it might only want to do it when you first type the table name from my read of https://github.com/codemirror/lang-sql/blob/ebf115fffdbe07f91465ccbd82868c587f8182bc/test/test-complete.ts#L37. It's possible I'm just passing something wrong, but it may end up being something that needs feature work upstream. |
Deployed a fresh copy:
|
Have you ever seen CodeMirror correctly auto-completing columns? I'm not entirely sure I believe that the feature works anywhere else. |
Codecov ReportBase: 92.55% // Head: 92.55% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1893 +/- ##
=======================================
Coverage 92.55% 92.55%
=======================================
Files 35 35
Lines 4432 4432
=======================================
Hits 4102 4102
Misses 330 330 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
The default focus styles appear to be
Which I also see on desktop. Would be nice to changed to whatever the default UA textarea styles are to blend in better but I wouldn't recommend removing it entirely - just to keep the visual indication that the element is focused. Maybe followup material to have a theming pass |
Nice. And is it possible to include another field which is an escaped table name (only when necessary) - i.e. |
Or I guess you could return only the escaped table name and then we could derive the unescaped from the client side (removing the outer |
Alright, added Cmd+Enter to submit (Ctrl+Enter on Windows as well bc of using Meta-Enter on codemirror). We can make that MacOS only by changing the combo to Cmd+Enter specifically but I think it's probably fine to have both. |
I think the table completion still has some quirks to work out. Something like
Seems to work alright, although it will append it after any other numbers you've started typing - so you end up with You can do
Which is pretty neat and will show the non-escaped string but complete to the escaped one. You can't easily do that with the table names themselves (you can pass a It's buggy enough (bad output for these unusual table names) that I'd suggest that work gets moved into a follow up to the upgrade to 6. That would give space to sort out how to deliver that to the view directly, figure out where name escaping should happen, and have overall testing to uncover bugs and fix papercuts before enabling it. |
Honestly I'm not too bothered if table names with weird characters don't work correctly here - I care about those in the Datasette |
I can push up a commit that uses the static fixtures schema for testing, but given that the query used to generate it is authed we would still need some work to make that work on live data, right? Ideally it could come down to db and query views directly to avoid waiting on an extra xhr and managing that state change.On Nov 16, 2022, at 2:16 PM, Simon Willison ***@***.***> wrote:
Honestly I'm not too bothered if table names with weird characters don't work correctly here - I care about those in the Datasette fixtures.db database because Datasette aims to support ANY valid SQLite database, so I need stuff in the test suite that includes weird edge cases like this. But I would hope very few people actually create tables with spaces in their names, so it's not a huge concern to me if autocompletion doesn't work properly for those.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Yeah, push that up. I'm happy to wire in the query right after we land this. |
Alright with f254be4 we should be getting autocomplete on fixture data. Give that a test and see what you think |
Deployed that to https://datasette-pr-1893.vercel.app/fixtures - looks good to me! |
OK, let's do it! Thanks so much for this. |
I'll open a follow-up issue to fix the schema. |
Should we empty out the fixture schema to avoid fixture autocomplete showing up on live databases in the interim, or are you planning to tackle #1897 shortly? |
I'm going to tackle #1897 in the next few minutes. Tests failed due to Prettier check, just pushed a fix so it would ignore |
* Upgrade to CodeMirror 6 * Update contributing docs * Change how resizing works * Define a custom SQLite autocomplete dialect * Add meta-enter to submit * Add fixture schema for testing
In an effort to get closer to table / column autocomplete I took a shot at #1796. I haven't done a lot of testing but would be curious if this fixes some of the concerns raised in #1796 (comment) for example.
Done:
Not done:
📚 Documentation preview 📚: https://datasette--1893.org.readthedocs.build/en/1893/