Skip to content
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

Filters no longer statically built into the binary, are dynamically loaded in JS #37

Closed
wants to merge 3 commits into from

Conversation

petertrotman
Copy link
Contributor

Just a proof of concept for now - should solve the compile times issue since the wasm can be compiled once and then filters generated separately.
To test, build the project as normal then overwrite the .wasm files by running (in the engine dir):

tinysearch/engine $ wasm-pack build --target web --release --out-dir ../

Next steps:

  1. Separate out the wasm from the filter generation, publish to npm
  2. Build a cli for generating the filters

Let me know if you think it's worth continuing

Cheers,

P

@mre
Copy link
Member

mre commented Oct 24, 2019

Thanks for your PR.

I like your approach; it's quite smart.
On the other side, I like how we provide a single, static binary with all search functionality built-in.

The builds are much faster now thanks to the work by @CephalonRho in #13. That's only for subsequent builds, though. The initial build is still slow, but my main focus right now is payload size.
The filters have to be loaded anyway, so we won't reduce that with this PR.

Maybe we could provide a flag, which will leave the choice of dynamically loading the filters to the user?
@CephalonRho, what is your perspective?

@petertrotman
Copy link
Contributor Author

I understand what you're saying, that's fine. I think if I were to use it on my sites I would prefer to use it as an npm-installed static package with some bundled script to create the filter manifest file, so I think that is still worth pursuing at some point - I will progress it when I have a bit of time.

I reckon the easiest way would be to split it out into a separate package within the workspace. I'll keep working on my dynamic-filters branch so we can keep an eye on progress here.

Cheers,

P

@mre
Copy link
Member

mre commented Oct 24, 2019

Thanks. Shipping it to npm would be great, yeah. I haven't thought about that when developing the tool - probably because I don't use npm too often myself.
Happy to revisit this topic again if we get a working prototype up and running. So just keep on pushing to this branch if you find the time and we can check back later. 👍 Thanks for your efforts so far.

@shuni64
Copy link
Contributor

shuni64 commented Oct 24, 2019

Seems like this is done for now, but I'll still go ahead and add my comment here.

Dynamically loading the filters instead of including them in the binary won't really improve the compile times, but of course not having to compile the binary at all when only the filters change is best.

In my opinion the biggest benefit of this is, as @petertrotman suggested, being able to distribute precompiled binaries through npm. This would allow for much better integration with the JavaScript ecosystem.
A big disadvantage would be breaking existing setups when updating to the new version, so maybe there should be a feature to keep the previous functionality.

Still a good idea that should at least be an opt-in feature sometime.

@mre mre added this to Backlog in Roadmap to v1.0 Jun 24, 2020
@mre
Copy link
Member

mre commented Sep 27, 2022

This was a really great effort, but as time has shown I never got around to pushing it over the finishing line. That's unfortunate but such is life. Nevertheless, thanks a lot for all the efforts. 👍

@mre mre closed this Sep 27, 2022
@millette millette mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Roadmap to v1.0
  
Backlog
Development

Successfully merging this pull request may close these issues.

None yet

3 participants