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

refactor: replace opentype with harfbuzz #560

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LuciNyan
Copy link
Contributor

@LuciNyan LuciNyan commented Oct 7, 2023

WIP

Description

  • Replace opentype with harfbuzz.
  • Since harfbuzz only provides WASM officially, inline the WASM file as base64 in the JS file to keep usage simple and concise.
  • The harfbuzz package contains hb.wasm but does not export it. Currently I am directly copying the file into our project, but how should I directly use its hb.wasm during bundling? (This should just be a build issue).

@vercel
Copy link

vercel bot commented Oct 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
satori-playground ❌ Failed (Inspect) Oct 7, 2023 10:34am

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@Jackie1210
Copy link
Contributor

Jackie1210 commented Oct 7, 2023

Wow, I also have been thinking of bringing harfbuzz to satori and I want to share my opinions about it.

  1. make harfbuzz as a separated npm pkg, so we ought to make this repo a monorepo for some reasons:
    1. make satori logic clear
    2. We can track upstream and test harfbuzz indepdently if we come across unknow issue(e.g. we can make a harfbuzz's playgroud, since official playgroud is cheap)
    3. build harfbuzz source code by emscripten for APIs, which satori really needs to make size smaller
  2. base on 1, we should write some tests to keep it stable even if harfbuzz is ABI stable.
  3. Maybe you can refer to yoga-wasm-web

The harfbuzz package contains hb.wasm but does not export it. Currently I am directly copying the file into our project, but how should I directly use its hb.wasm during bundling

  1. I don't think inline it as base64 is a good idea because of size. Base on 1, we could import it dynamically.

Since harfbuzz only provides WASM officially, inline the WASM file as base64 in the JS file to keep usage simple and concise.

@LuciNyan
Copy link
Contributor Author

LuciNyan commented Oct 7, 2023

Wow, I also have been thinking of bringing harfbuzz to satori and I want to share my opinions about it.

  1. make harfbuzz as a separated npm pkg, so we ought to make this repo a monorepo for some reasons:

    1. make satori logic clear
    2. We can track upstream and test harfbuzz indepdently if we come across unknow issue(e.g. we can make a harfbuzz's playgroud, since official playgroud is cheap)
    3. build harfbuzz source code by emscripten for APIs, which satori really needs to make size smaller
  2. base on 1, we should write some tests to keep it stable even if harfbuzz is ABI stable.

  3. Maybe you can refer to yoga-wasm-web

The harfbuzz package contains hb.wasm but does not export it. Currently I am directly copying the file into our project, but how should I directly use its hb.wasm during bundling

  1. I don't think inline it as base64 is a good idea because of size. Base on 1, we could import it dynamically.

Since harfbuzz only provides WASM officially, inline the WASM file as base64 in the JS file to keep usage simple and concise.

I'm really glad to get your feedback, but I have some confusion and want to make sure I understand correctly:

  • First, I'm not sure why we need to publish a separate harfbuzz npm package, because there is already an official harfbuzzjs package for JS which provides the wasm file and some wrappers. All we would be doing is making some targeted changes to the wrapper interfaces to suit satori's needs (which I don't expect to be too extensive). Could you elaborate on what exactly this proposed harfbuzz npm pkg would contain?

  • If we don't inline the wasm as base64 directly into the JS, web users would need to load this separate wasm file on the page. I'm not certain about the best approach here.

I really appreciate your valuable insights, but think this part needs some discussion. Perhaps we can get @shuding 's opinions as well. xD

@shuding
Copy link
Member

shuding commented Oct 7, 2023

Super excited to see this proof of work, this will fix a lot of tricky issues in Satori! Thanks for the great work as always @LuciNyan!

Currently I am directly copying the file into our project

I think that's fine for now just ensure we're copying the LICENSE files as well. We do that for fonts and other things in the past (and in Next.js, too).

Regarding inlining the WASM file, I think it's a compromise if we don't want to break the current API of satori. Unlike satori/wasm, satori includes Yoga's Node.js native/asm.js which doesn't need the user to do extra work to load a file manually in Node.js or browser. In general I think it's fine to land experimental APIs and have some changes before 1.0, we can always do that! Maybe instead of base64, we inline it into a Uint8Array (i.e. https://github.com/ballercat/wasm-loader/blob/master/index.js) which might have a higher compression rate. That said I'm open to any good ideas here, breaking the current API is also fine if that's better.

Re: make this repo a monorepo, definitely! I have the plan to move vercel/og here and setup monorepo, but didn't get enough time. Maybe I'll start a draft PR first. We'll need many subpackages for sure in the future.

Re: maintaining a separate Harfbuzz build. This will solve the asm.js thing but I have two concerns. One is that I don't know about how much effort it will be if we expand the scope, maybe that's not difficult (e.g. setting up Harfbuzz builds, publishing it, keeping up with the upstream). Another concern is that maybe we should slowly move away from asm.js and always have users to load WASM files for performance reasons.

Re: a separate playground for Harfbuzz. Maybe we can have a couple of examples in our existing playground showing different scripts and shapes? And redesign the playground a bit, like making it a full working app with file storage in local cache, WYSIWYG, exporting Next.js/Node code for your design, etc. like an in-browser PS. That would be amazing and help more people I believe!

Thanks again!

@Jackie1210
Copy link
Contributor

hahaha, harfbuzz is a big killer, which has more feature which harfbuzzjs does not support. As far as I know, harfbuzzjs only does basic work and does not support esm and slow feedback for outer contribution like my PR month's ago(😼) and not friendly API design(?). Yeah, we can land harfbuzz at first and then think about the best practice. Anyway, cheer for your great work! @LuciNyan

First, I'm not sure why we need to publish a separate harfbuzz npm package, because there is already an official harfbuzzjs package for JS which provides the wasm file and some wrappers. All we would be doing is making some targeted changes to the wrapper interfaces to suit satori's needs (which I don't expect to be too extensive). Could you elaborate on what exactly this proposed harfbuzz npm pkg would contain

@Jackie1210
Copy link
Contributor

an in-browser PS? Amazing! Let's rollup. 👻

Re: a separate playground for Harfbuzz. Maybe we can have a couple of examples in our existing playground showing different scripts and shapes? And redesign the playground a bit, like making it a full working app with file storage in local cache, WYSIWYG, exporting Next.js/Node code for your design, etc. like an in-browser PS. That would be amazing and help more people I believe!

@shuding
Copy link
Member

shuding commented Oct 7, 2023

hahaha, harfbuzz is a big killer, which has more feature which harfbuzzjs does not support. As far as I know, harfbuzzjs only does basic work and does not support esm and slow feedback for outer contribution like my harfbuzz/harfbuzzjs#90 month's ago(😼) and not friendly API design(?).

Wow, this is something I didn't know and thanks for the context! Then I'm totally OK if maintaining a custom build makes our lives easier!

@LuciNyan
Copy link
Contributor Author

LuciNyan commented Oct 7, 2023

contribution like my PR month's ago(😼)

COOOOOOOLLLL!!!

@yisibl
Copy link
Contributor

yisibl commented Oct 16, 2023

What are the main problems that harfbuzzjs was introduced to solve here?

@LuciNyan
Copy link
Contributor Author

What are the main problems that harfbuzzjs was introduced to solve here?

The main rendering issues that harfbuzzjs aims to address involve scripts using ligatures. Like:
#215
#516

@yisibl
Copy link
Contributor

yisibl commented Oct 18, 2023

@LuciNyan Yeah, this is the way.
image

@ahmedriad1
Copy link

Any updates? This PR is amazing and it resolves the only dealbreaker with satori. I would much prefer satori over puppeteer lol

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.

5 participants