Skip to content

Conversation

@etiennenoel
Copy link
Contributor

… with the worker.

@etiennenoel
Copy link
Contributor Author

@tomayac if you can review the code and let me know what you think. I've updated the demo. I'm not sure about hardcoding the link to the comlink cdn but the alternative is to package it somehow and I don't think that's better either.

Also, I will update the readme.md once (and if) we are satistified with the code.

@tomayac
Copy link
Collaborator

tomayac commented Jun 26, 2023

Thanks, this is great, and allows people to either use this convenience layer, or ignore it and use the traditional approach.

Regarding the Comlink dependency, just npm install it, and bundlers will be smart about only including it if it’s actually needed. For the demo, hard coding a CDN concretely is fine, but for the src/ code you want to avoid hard coded dependencies.

Copy link
Collaborator

@tomayac tomayac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there. I like where this is headed. Thanks for working on this!


console.log(rows);

document.getElementById('sqlite-client').innerHTML =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use a template string here and nest it properly?

@etiennenoel
Copy link
Contributor Author

@tomayac I understand not including dependencies in the /src folder. In the case of the sqlite-worker.mjs, if I include a normal npm dependency as import {comlink} from "comlink", I will have to bundle it somehow to make the demo work.

What's the easiest way I should do this you think? (trying to avoid including too many dependencies in here).

@socket-security
Copy link

New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives1 Size Publisher
comlink 🆕 4.4.1 None +0 248 kB benjamind

Footnotes

  1. https://docs.socket.dev

@tomayac tomayac marked this pull request as ready for review June 30, 2023 14:34
@tomayac tomayac merged commit d713ca1 into sqlite:main Jun 30, 2023
@schickling
Copy link
Collaborator

schickling commented Jul 4, 2023

Sorry for the late response and thanks for this PR. As a early user of this library (and given I've built a predecessor of this project), I'd like to voice a concern in regards to adding comlink as a dependency:

I've used comlink quite a bit in previous projects, and while it certainly provides some convenience when working with workers, it also adds a considerable amount of performance (and also bundle size) overhead. Unfortunately I don't have any kind of benchmark numbers available right now, but it was significant enough for my use cases, that I've removed comlink where it became a bottleneck. (Btw still kudos to the folks who've created comlink as it definitely helps when getting started with workers in the web.)

Given sqlite-wasm is meant to be used (to my knowledge) as a fairly low-level wrapper around SQLite's WASM build, I feel pretty strongly about trying to avoid further dependencies and performance overhead however possible.

Happy to chat more about this.


Side comment: the merged source file is currently importing from unpkg and not from the locally installed dependency:

CleanShot 2023-07-04 at 12 15 39@2x

@tomayac
Copy link
Collaborator

tomayac commented Jul 4, 2023

Thanks for chiming in. I’ve made changes to the finally released version that imports locally, and not from the CDN. Comlink is used as a convenience layer for those who like it, the original way of using the library is still available unchanged.

@schickling
Copy link
Collaborator

Comlink is used as a convenience layer for those who like it, the original way of using the library is still available unchanged.

That's good to know. Would you be open to making comlink an optional dependency to keep the core of the package minimal? Another approach could be to create a separate package with the new convenience layer.

@tomayac
Copy link
Collaborator

tomayac commented Jul 5, 2023

We can make Comlink an optional dependency, sure. Can you please open a quick PR? I’m OoO until the 17th, but happy to merge it as soon as I return.

@etiennenoel
Copy link
Contributor Author

I've created one here: #20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants