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

fix: add special target for Cloudflare Workers #151

Merged
merged 11 commits into from
Jan 8, 2024
Merged

Conversation

mhmd-azeez
Copy link
Contributor

Fixes #150

Unfortunately, from what I see it's not possible to have one package that works for boths Cloudflare Workers and other platforms. The problem is, Cloudflare Workers need you to import the wasm statically (dynamic import doesn't work), and the other platforms don't let you import wasm files statically.

@chrisdickinson
Copy link
Contributor

I haven't looked too closely at how observe-sdk is packaged up yet, but from #150 the observe-sdk appears to use wasm-bindgen?

This might retread ground you've already covered, but it looks like Cloudflare has some instructions on how to patch wasm-bindgen output to be used in a worker. Also: I believe that wrangler respects {"exports": {"workerd": {...}} in package.json (like the bun and node mappings in the js-sdk). That might let us unify the packages?

@mhmd-azeez
Copy link
Contributor Author

@chrisdickinson two great pointers! I will take a look at them both. thank you very much! I was very unhappy with the fact that I had to create new packages, but this gives me a lot of hope

@mhmd-azeez mhmd-azeez changed the title fix: create a new package for workers fix: add special target for Cloudflare Workers Jan 3, 2024
@mhmd-azeez mhmd-azeez marked this pull request as ready for review January 3, 2024 13:08
Copy link
Contributor

@chrisdickinson chrisdickinson left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@nilslice
Copy link
Member

nilslice commented Jan 4, 2024

@mhmd-azeez - nice! were you able to test this on a remote worker deployed in Cloudflare? or only workerd locally?

@mhmd-azeez
Copy link
Contributor Author

@nilslice I had only tested locally because I assumed that the npm packages would be installed on the server, but it seems like when you do wrangler deploy, it just copies everything. I can confirm that it works on Cloudflare too!

@mhmd-azeez mhmd-azeez merged commit 2806fe4 into main Jan 8, 2024
6 checks passed
@mhmd-azeez mhmd-azeez deleted the fix-workers branch January 8, 2024 11:51
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.

bug: JS SDK doesn't work in Cloudflare workers
3 participants