feat (toolbar): Adding new top level package for web developer toolbar #446

paulharter · 2023-09-19T20:58:23Z

No description provided.

toolbar/package.json

thruflo · 2023-09-20T07:22:07Z

Just to note on the packaging stuff, does it make sense to have the example packaging in the toolbar folder? As opposed to keep the toolbar a lighter package and include it as a dependency in an/all-of-the example(s)?

Or if we need a proper app to dev/test the toolbar, maybe that goes in a subfolder that isn't included in the package list?

netlify · 2023-09-20T09:36:50Z

❌ Deploy Preview for electric-sql-basic-items-example failed.

Name	Link
🔨 Latest commit	`1c6cb94`
🔍 Latest deploy log	https://app.netlify.com/sites/electric-sql-basic-items-example/deploys/650abd05db1ae400086e1fe8

paulharter · 2023-09-20T09:45:14Z

Just to note on the packaging stuff, does it make sense to have the example packaging in the toolbar folder? As opposed to keep the toolbar a lighter package and include it as a dependency in an/all-of-the example(s)?

Or if we need a proper app to dev/test the toolbar, maybe that goes in a subfolder that isn't included in the package list?

This is how it works. The web app in the folder is for dev only and isn't built into the package. The package itself is quite minimal and can be added to any of the examples. I can try to tease apart the dev app code from the toolbar code for clarity. Maybe I can get code reloading etc to work properly with a clean example app and the toolbar as a dependency then I could dump the dev web app altogether. I tried this before and couldn't get it to work.

maybe using npm link will work - I'll try this

paulharter · 2023-09-25T09:08:02Z

I've moved the dev app into a sub folder for tidiness but the structure is the same. There is a dev app that is used during development in dev-app but the only stuff included in the build into dist is the actual toolbar stuff.

I have added the toolbar to the web-wa-sqlite example. It shows if you do export DEBUG_MODE=true . At the moment it is included with a relative file reference in the devDependencies. At some point we need to add this package to our automated build for node. After talking to @kevin-dp I think all thats required is to add an initial build to out npm registry and the current machinery will then pick it up.

paulharter · 2023-09-28T17:09:00Z

@thruflo I've got the SQLite console working with codemirror. And sort of got the running of DAL queries working too but was blocked by the way useElectric is written, so I've parked that for now.

There are a couple of outstanding build issues:

The lib I used to get codemirror working with React 18 has a bad use of global which I'm having to patch in the build script until I can work round it better
For some reason I can't fathom the the export of the in the component is broken but my original adding it with a function works ok. I'll try to fix this next week.

We could merge this now with these two issues or wait til fix them properly next week.

thruflo · 2023-09-28T17:26:39Z

Awesome :) I'll have a play with it. Sounds like it's worth resolving the build issues before merging -- no mad rush.

thruflo · 2023-09-28T17:28:06Z

(Maybe also sync on the useElectric thing -- I'm curious)

examples/web-wa-sqlite/package.json

thruflo · 2023-09-29T09:02:10Z

examples/web-wa-sqlite/src/Example.tsx

 import './Example.css'

+// toolbar imports


Ideally it would be good to keep the imports in this example as clean / minimal as possible. I'd suggest add a toolbar.tsx src file and have that encapsulate these details and just expose the component.

Also presumably the css import should not be required here (should be imported and thus bundled by the toolbar lib).

Also you're mixing double quote and single quite and the import spacing is inconsistent. Syntax wise you want:

import { globalRegistry } from 'electric-sql/satellite' import { AddToolbar, TypescriptApi } from '@electric-sql/debug-toolbar'

This should be normalised by prettier, so have you run the listing here?

Accessing the globalRegistry needs to be done from the client code not the library I played with this a lot - its to do with when to global singleton gets created

I believe @thruflo was hinting at having a single file expose all of this such that you need only a single import:

import { globalRegistry, AddToolbar, TypescriptApi } from '@electric-sql/debug-toolbar'

For the rest nothing changes.

examples/web-wa-sqlite/src/Example.tsx

examples/web-wa-sqlite/src/index.tsx

toolbar/src/client/api/api-typescript.ts

thruflo · 2023-09-29T09:33:59Z

toolbar/src/index.css

@@ -0,0 +1,160 @@
+#electric-toolbar {


I guess it makes sense to only have one toolbar at a time and for it to always be at the bottom. But does this need to be the concern of the core toolbar component, i.e.: could the toolbar have normal positioning and then there's a wrapper that deals with the fixed / absolute etc.?

This is useful to get the css reset working nicely - this id specificity overrides class ones

toolbar/src/index.css

toolbar/src/index.tsx

thruflo · 2023-09-29T09:42:38Z

Note from ppm install at repo root:

toolbar
└─┬ react-codemirror2 7.2.1
  └── ✕ unmet peer react@">=15.5 <=16.x": found 18.2.0

Seems react-codemirror2 expects React unto 16.x.

thruflo · 2023-09-29T09:44:04Z

There's a missing zod dev rep:

➜  toolbar git:(paul/toolbar) ✗ yarn start
yarn run v1.22.19
warning ../../../../package.json: No license field
$ SERVE=true npm run build-dev

> @electric-sql/[email protected] build-dev
> node ./dev-app/copy-wasm-files.js && node dev-app/builder.js

✘ [ERROR] Could not resolve "zod"

    dev-app/src/generated/client/index.ts:1:18:
      1 │ import { z } from 'zod';
        ╵                   ~~~~~

  You can mark the path "zod" as external to exclude it from the bundle, which will remove this
  error.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
➜  toolbar git:(paul/toolbar) ✗

thruflo · 2023-09-29T09:49:17Z

I get an error trying to edit the SQL command. Specifically, I can move the cursor around but then when I try to e.g.: type backspace to delete a character I get:

[Error] TypeError: string.split is not a function. (In 'string.split(/\r\n?|\n/)', 'string.split' is undefined)
	(anonymous function) (index.js:18585)
	callCallback2 (index.js:5600)
	dispatchEvent
	invokeGuardedCallbackDev (index.js:5625)
	invokeGuardedCallback (index.js:5659)
	reportUncaughtErrorInDEV (index.js:18584)
	captureCommitPhaseError (index.js:21480)
	commitLayoutMountEffects_complete (index.js:19904)
	commitLayoutEffects_begin (index.js:19891)
	commitLayoutEffects (index.js:19842)
	commitRootImpl (index.js:21275)
	commitRoot (index.js:21199)
	performSyncWorkOnRoot (index.js:20817)
	performSyncWorkOnRoot
	flushSyncCallbacks (index.js:11061)
	(anonymous function) (index.js:20549)
[Error] The above error occurred in the <Controlled2> component:

Controlled2@http://localhost:3002/index.js:48143:32
div
div
div
SQLTab@http://localhost:3002/index.js:67439:25
div
div
ToolbarTabs@http://localhost:3002/index.js:67561:30
div
ElectricToolbar@http://localhost:3002/index.js:67641:31

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://reactjs.org/link/error-boundaries to learn more about error boundaries.
	logCapturedError (index.js:15962)
	(anonymous function) (index.js:15982)
	callCallback (index.js:12437)
	commitUpdateQueue (index.js:12454)
	commitLayoutEffectOnFiber (index.js:19009)
	commitLayoutMountEffects_complete (index.js:19902)
	commitLayoutEffects_begin (index.js:19891)
	commitLayoutEffects (index.js:19842)
	commitRootImpl (index.js:21275)
	commitRoot (index.js:21199)
	performSyncWorkOnRoot (index.js:20817)
	performSyncWorkOnRoot
	flushSyncCallbacks (index.js:11061)
	(anonymous function) (index.js:20549)
[Error] TypeError: string.split is not a function. (In 'string.split(/\r\n?|\n/)', 'string.split' is undefined)
	flushSyncCallbacks (index.js:11071)
	(anonymous function) (index.js:20549)

thruflo · 2023-09-29T09:55:55Z

I have a few comments on the layout and naming:

status, could perhaps be connection; and would be great to use the connectivity toggle here to allow user to toggle on and off
IndexedDB, firstly this is web driver config specific; secondly maybe we want to call this Reset; thirdly, perhaps connectivity, reset, re-sync, clear local changes, etc. could all be in a single panel together
SQLite, not quite sure of the right name, maybe shell? on the layout / results, I think we want a nice table view for the results with column names; this really needs full width, so I think the input box has to be above the results, unless maybe at very large widths

On the last point, generally we want this to work with mobile too, so we have to take care of how functional the UI is at smaller sizes. And it might be nice to make resizable -- although I know the extension panel will handle this so maybe ignore. But right now as an overlay, it hides the page content underneath it, rather than allowing that to scroll.

paulharter · 2023-10-05T08:46:05Z

@thruflo can you have another look at this now? It's still using the separate root element added into the dom. I couldn't get it to build as an exported react component, due to complexities of loading react. (The codemirror react library I'm using has a require("react") which makes me marking react as external causes a runtime error for dynamic imports, but if I don't I get issues with two reacts clashing and disallowing my hooks.)

I have published it to the npm registry yet - this might have to wait til next week.

kevin-dp

Great work!
I left quite some comments but nothing major.
Also, prettier reports some formatting issues when running npm run check-styleguide both in the toolbar and in linearlite.

examples/linearlite/src/App.tsx

kevin-dp · 2023-10-06T06:48:56Z

examples/linearlite/src/App.tsx

@@ -11,6 +11,11 @@ import LeftMenu from './components/LeftMenu'
 import { ElectricProvider, initElectric, dbName, DEBUG } from './electric'
 import { Electric } from './generated/client'

+// toolbar imports
+import { globalRegistry } from "electric-sql/satellite";
+import AddToolbar, { TypescriptApi } from '@electric-sql/debug-toolbar'


Could we find a more descriptive name for TypescriptApi?
Is it just the toolbar API itself? Could be Toolbar

I've gone with ToolbarTypescriptas there may be other

I don't really get why we are naming the toolbar class after the programming language its implemented in.
What do you mean when you say there could be another? If it is in another language it won't be in this package so it will be imported from somewhere else. I don't see the need to differentiate its name by the programming language.

examples/linearlite/src/App.tsx

examples/linearlite/package.json

kevin-dp · 2023-10-06T06:54:02Z

examples/web-wa-sqlite/src/Example.tsx

 import './Example.css'

+// toolbar imports


I believe @thruflo was hinting at having a single file expose all of this such that you need only a single import:

import { globalRegistry, AddToolbar, TypescriptApi } from '@electric-sql/debug-toolbar'

For the rest nothing changes.

toolbar/src/index.tsx

toolbar/src/tabs.tsx

toolbar/.gitignore

kevin-dp · 2023-10-06T08:03:27Z

toolbar/package.json

+  "types": "dist/index.d.ts",
+  "author": "ElectricSQL",
+  "license": "Apache-2.0",
+  "scripts": {


Agree. I would even argue that we don't want to package a demo-app with the toolbar. Demo apps should be found in examples.

paulharter · 2023-10-11T10:20:53Z

@kevin-dp I've pretty much done all the changes as you suggested except:

I couldn't get the globalRegistry import from the toolbar code so am still passing it in
I called the api for the typescript client ToolbarTypescript rather than Toolbar

I have also added the OpenSourceOne font via a CDN

I tried using the pnpm workspace for local dev but you still have go through the build step for every change, it usable but annoying. I have added instructions on setting this up locally in the readme, but I will keep my local dev-app for convenience.

kevin-dp

I left some new comments.
Also, it seems that there are some comments you did not address (e.g. the comments in toolbar/src/index.tsx).

Also i just noticed that there are no unit tests.
Perhaps we should add unit tests for the toolbar to check that when you create a toolbar it can access the satellites, and correctly query and reset the DB. We can use the better-sqlite3 driver in these unit tests. That will give some assurance that the global registry mechanism works.

There is a conflict on the lock file so you will need to update it and push it.

Don't forget to generate a changeset for your PR.

kevin-dp · 2023-10-16T09:36:52Z

toolbar/package.json

+    "@types/node": ">=16.11.0",
+    "@types/react": "^18.0.18",
+    "@types/react-dom": "^18.0.11",
+    "electric-sql": "latest",


Is this a real dev dependency?
It's probably a remnant of the local dev app you use.
But since we don't checkout the dev app we probably don't need this dev dependency.

I think it need them all - at least it doesn't build without them.

in src/api/toolbar-typescript.ts the global registry is being imported:

import { GlobalRegistry } from 'electric-sql/satellite'

So electric-sql needs to be a dependency rather than a dev dependency.
Perhaps it should even be a peer dependency.

Noticing now that it is listed as a peer-dependency. Why does it need to be a dev dependency also then? Is it being used in some test somewhere? I don't think there are any tests.

kevin-dp · 2023-10-16T09:41:37Z

toolbar/src/api/types.ts

@@ -0,0 +1 @@
+export type { Row, Statement, ConnectivityState } from 'electric-sql/dist/util'


We should not import/re-export things from electric-sql/dist, dist is the distribution folder.
This should be electric-sql/util. I would not bother re-exporting it here, you can import it directly from electric in whichever file you need it. Please double check that there are no other places where you import from electric-sql/dist.

I've done these import directly now

but you're still doing import { Row, Statement, ConnectivityState } from 'electric-sql/dist/util'.
You should not import from dist but just electric-sql/util. Same for other imports where you import from dist.

kevin-dp · 2023-10-16T09:45:12Z

toolbar/src/index.tsx

+  }
+}
+
+export function typescriptApi(


i don't like typescript being in the name of this function.

I've changed this to clientApi

kevin-dp · 2023-10-23T10:17:48Z

toolbar/package.json

@@ -28,7 +28,8 @@
    "esbuild": "^0.16.17",
    "esbuild-plugin-inline-image": "^0.0.9",
    "prettier": "3.0.3",
-    "typescript": "^4.4.3"
+    "typescript": "^4.4.3",
+    "zod": "^3.21.1"


Why do we need zod as a dev dependency?
I don't see it being used anywhere in the toolbar.

no you're right

paulharter · 2023-11-29T11:26:02Z

#686

icehaunter reviewed Sep 20, 2023

View reviewed changes

toolbar/package.json Show resolved Hide resolved

paulharter marked this pull request as draft September 25, 2023 15:12

paulharter force-pushed the paul/toolbar branch from fdbaf26 to 7999d39 Compare September 25, 2023 15:43

paulharter marked this pull request as ready for review September 28, 2023 16:59

paulharter requested a review from thruflo September 28, 2023 17:00

thruflo requested changes Sep 29, 2023

View reviewed changes

kevin-dp requested changes Oct 6, 2023

View reviewed changes

kevin-dp requested changes Oct 16, 2023

View reviewed changes

kevin-dp reviewed Oct 23, 2023

View reviewed changes

paulharter added 8 commits October 23, 2023 11:20

toolbar in its own package

11befac

toolbar in top level package

4eab955

adding prettier

fc862e8

moving toolbar dev app into subfolder

7b5299b

wip on autolpading toolbar

29b04e2

linting

11e48ec

sql console

0e3b19b

with sqlite console

98f0807

paulharter and others added 11 commits October 23, 2023 11:22

linting and fixing codemirror

90e9cf6

fix zod install for pnpm

f18c963

tidy up stray Promise

eabbb3d

whitespace

efb5817

chore: add CI for styleguide and type checks

92ccfa8

examples ref latest toolbar

f1752ea

removing toolbar dev app

8b7aa8e

tidy up api and minor refactors

67bd757

adding back zod to deps

8590b41

removing zod again

bf0ad7d

direct import of electric client types

c54a42a

paulharter force-pushed the paul/toolbar branch from 859c00d to c54a42a Compare October 23, 2023 10:32

paulharter added 3 commits October 23, 2023 11:51

renaming typescriptApi to clientApi

42667aa

removing toolbar from examples

94b6b44

linting

17dee6d

paulharter marked this pull request as draft November 20, 2023 16:00

paulharter closed this Nov 29, 2023

		@@ -0,0 +1 @@
		export type { Row, Statement, ConnectivityState } from 'electric-sql/dist/util'

feat (toolbar): Adding new top level package for web developer toolbar #446

feat (toolbar): Adding new top level package for web developer toolbar #446

Conversation

paulharter commented Sep 19, 2023

thruflo commented Sep 20, 2023

netlify bot commented Sep 20, 2023

❌ Deploy Preview for electric-sql-basic-items-example failed.

paulharter commented Sep 20, 2023 • edited Loading

paulharter commented Sep 25, 2023

paulharter commented Sep 28, 2023

thruflo commented Sep 28, 2023

thruflo commented Sep 28, 2023

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

thruflo commented Sep 29, 2023

thruflo commented Sep 29, 2023

thruflo commented Sep 29, 2023

thruflo commented Sep 29, 2023

paulharter commented Oct 5, 2023

kevin-dp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-dp Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulharter commented Oct 11, 2023

kevin-dp 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

kevin-dp Oct 23, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulharter commented Nov 29, 2023

paulharter commented Sep 20, 2023 •

edited

Loading

kevin-dp Oct 16, 2023 •

edited

Loading

kevin-dp left a comment •

edited

Loading

kevin-dp Oct 23, 2023 •

edited

Loading