solid js library #37
solid js library #37
Conversation
.vscode/settings.json
Outdated
| @@ -22,5 +22,19 @@ | |||
| "editor.defaultFormatter": "biomejs.biome", | |||
| "editor.tabSize": 2, | |||
| "editor.insertSpaces": true | |||
| }, | |||
| "workbench.colorCustomizations": { | |||
| "[Monokai Pro]": { | |||
There was a problem hiding this comment.
not sure where these came from lol
lib/sqlsync-solid-js/src/sqlsync.ts
Outdated
There was a problem hiding this comment.
This could be a shared file or library between the Solid and React implementations.
lib/sqlsync-solid-js/src/sql.ts
Outdated
There was a problem hiding this comment.
This could be part of the shared JS library
lib/sqlsync-solid-js/src/util.ts
Outdated
There was a problem hiding this comment.
This could also be part of the shared JS library
|
Wow Matt! This looks awesome. I agree, I think that a ton of code needs to be refactored out into a shared js lib for the different frameworks. I didn't realize how similar the react and solid implementations would be - that's good to know! Appreciate the work here, will get back to this PR asap once I have a chance to play around with it and think about how I want to refactor things. |
Thanks! Yeah, Solid JS and React APIs are very similar. I have a bit of cleanup to do and will mark as ready for review. I should get around to it this afternoon. |
@carlsverre should be ready for a review. As noted in the description, I didn't run the test locally. Not sure how I should go about building and running the sanity test without investing more effort. I can confirm that it's woking in my downstream demo application. I'm about to fold it into my production application so will let you know if I run into any issues there. |
|
As an aside, I'm excited to use and contribute to this project. The first two items on my mind are pluggable auth and storage but I don't know the specifics around what I need just yet. I should know more soon as I start to integrate it into my application. |
|
@matthewgapp No worries on testing, I'll do that as part of integrating this. One thought I had is whether merging all the framework libraries into one would be better? With modern JS build systems, the unused library code should be ignored pretty well. That way I just have different imports for each framework but it would be easier to share code that's similar. The other option is what you said above, which would be to refactor out the core SQLSync client class into a standalone library that each framework library depends on. I think that's probably the right approach, but curious what you think. |
I think it would depend on what your JS users would expect. I'm no expert on managing the all-so-tiring JS ecosystem but I'd err on the side of simplicity, both for authors and users. I think a thick JS core library and thin framework-specific wrapper approach aligns best against the goal of keeping things simple on the user side IMO. I'm not sure if a single library would be easier or worse for authors. However, separating the JS core from the framework libraries does ensure that the wrappers stay thin and decoupled, helping new framework authors get started without extensive knowledge of the JS core lib. |
matthewgapp
force-pushed
the
matt/feat/solid-js-library
branch
from
549144b
to
861ca94
Compare
|
@matthewgapp I just landed the react refactor. Can you rebase this on the main branch? For some reason the current git state of this branch also includes all my changes. |
|
also for testing, can you make a solidjs version of guestbook-react? you can also just re-use the existing reducer that's also in the examples directory. |
…o queries have been tested
matthewgapp
force-pushed
the
matt/feat/solid-js-library
branch
from
7ed121c
to
a72e0c5
Compare
@carlsverre implemented the guestbook and seems to be working. I noticed that you moved away from vite and to rollup for react library. How come? And would you like the solid js moved over to rollup too? |
|
It's not strictly needed, but I moved over for consistency. Since I moved the example out of sqlsync-react and into examples, it no longer seemed that vite was as needed. Rollup is slightly easier to control for libraries whereas vite seems better when you are shipping web apps. |
| }; | ||
|
|
||
| export const SQLSyncProvider: ParentComponent<Props> = (props) => { | ||
| const [sqlSync, setSQLSync] = createSignal<SQLSync>(createSqlSync(props)); |
There was a problem hiding this comment.
I'm not super familiar with how SolidJS works, but it appears that every time this component (SQLSyncProvider) renders it will re-create the SQLSync object. This is undesirable as each instance of the SQLSync object will initialize a connection to the shared worker at construction time. Is it possible to only call createSqlSync once if the signal has not yet been set?
There was a problem hiding this comment.
Solid JS calls component functions once, so this should correctly create the SQLSync object once.
See here for more info: https://www.solidjs.com/guides/getting-started#2-vanishing-components
lib/sqlsync-solid-js/src/hooks.ts
Outdated
| const [state, setState] = createSignal<QueryState<R>>({ state: "pending" }); | ||
|
|
||
| createEffect(() => { | ||
| let query = normalizeQuery(rawQuery()); |
There was a problem hiding this comment.
This can be a const. Also can you run biome on this PR? I'll add it to just soon, but in the meantime you can run:
pnpm exec biome check ./lib/sqlsync-solid-js/src
There was a problem hiding this comment.
"let" haha, i've written too much rust. Ran biome. Thanks for the tip. Would you mind adding instructions on biome to the CONTRIBUTING.md?
|
@carlsverre think i've addressed your comments. I moved the build from vite to rollup. Also fixed formatting but might not have caught everything. CI seems to be failing on unrelated LLVM stuff. |
Port of the React library for Soild JS. Have it working with a downstream demo app, which is also a port of the React SQLSync demo.
Todo
- The "sanity" test is copy pasta from the react directory but didn't take the time to build and run it. I do however have a downstream app (solid js port of the todo demo) consuming the solid js library so I can confirm that it appears to be working fine.