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

(draft) feat: use web workers for placeholder hash calculation #25

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

felixranesberger
Copy link
Contributor

@felixranesberger felixranesberger commented Aug 23, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSDoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@netlify
Copy link

netlify bot commented Aug 23, 2023

βœ… Deploy Preview for unlazy ready!

Name Link
πŸ”¨ Latest commit 8d3207a
πŸ” Latest deploy log https://app.netlify.com/sites/unlazy/deploys/64e6ef6c93f341000822f377
😎 Deploy Preview https://deploy-preview-25--unlazy.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@johannschopplich
Copy link
Owner

Thanks for the effort. There are many moving parts. I'm not sure what implications the async API has yet, since every framework integration depends on a sync API. Thus, it is a breaking change. I will take a deep dive later.

@felixranesberger
Copy link
Contributor Author

Currently I'm working on the web worker support for unbuild, because since a library is shipped, we have to add a rollup web worker plugin to the unbuild configuration.

Regarding the breaking async changes: I think it should mostly be fine, but I'll have a closer look, when I have time to work on the pull request further. Thanks for your first feedback πŸ‘

@johannschopplich
Copy link
Owner

Some food for thought on this PR:

  • Although I like the idea of using workers, a performance test would be great to get an idea of how big the gain really is.
  • We need to make sure the web workers work in Node.js, browsers and worker environments. Unlazy is specifically written to provide SSR support out of the box, which we need to maintain.
    • This is probably related to your unlazy web worker support implementation. Good luck with it!
  • Please also update the framework integrations, as the async placeholder generation would also require async watchers.

The bottom line is: I'm OK with the current synchronous implementation. I can only provide limited resources on the worker technology shift. But I'm still open to that one! It just has to be implemented airtight to not break user land.

@felixranesberger
Copy link
Contributor Author

felixranesberger commented Sep 19, 2023

I had some fun implementing the rollup plugin for web workers today, but to no success.
Switching from unbuild to vite for building the core package would probably fix the issue,
since vite offers built in support for web workers.

I'm not a huge fan, but I think for the moment it's the only way, I'm getting the web worker support to work. Will have a look at it maybe tomorrow, to see what actual performance benefits the web worker approach brings to the table πŸ‘ If the boost is high enough, we can look at some possible fixes for the issue.

@chrisspiegl
Copy link

This looks very promising and I appreciate the work being done on this πŸ‘. Thank you.

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