-
Notifications
You must be signed in to change notification settings - Fork 110
Try a different approach to importing "cloudflare:workers". #82
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
Conversation
Fixes #23 Per #23, many bundlers do not like our dynamic import of "cloudflare:workers". This PR takes a different approach, using the "exports" feature of "package.json" to point workerd at a different version of the code. That version imports "cloudflare:workers" and sticks it in the global scope before importing the rest of the library, thus allowing it to conditionally probe.
|
Missed this, will see over the weekend |
|
This pull request fixes all the issues I was having. Any idea when it will be merged and a new package created? |
|
looking at it today |
|
@scotthellewell Sorry for the delay on this and everything else Cap'n Web! In the month since Cap'n Web was released a whole lot of stuff has conspired to take of my time (and Sunil's too), and this week I'm on vacation. Next week I plan to circle back and address all the open Cap'n Web issues. |
|
@kentonv I hope you have a great vacation. I've grabbed your branch and compiled my own package from it for now which works. I just like to directly use from the npm repository when possible. But great library so far. |
threepointone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving this because it works and I want to unblock, but I think we can simplify it. I'll work on that myself later.
|
Awesome to see this is approved now. Do you have an estimate on when this will get merged and published to npm? I'd love to just reference your package, but I need this fix. If it will be a while still, I can publish a separate scoped npm package to use for the time being. But if it will be done in the next few days I would prefer to just wait. I would love an ETA. Thanks. |
|
Yes I'm planning to do a pass through all the open issues tomorrow and then probably push a release. Meant to do it yesterday but there was some other fire that came up. :/ |
* Add changeset for past PR #82. * Throw an error when attempting to access RpcTarget instance properties. This helps people learn why instance properties are not accessible over RPC, whereas returning `undefined` leaves them confused. Fixes #55 * Implement toString() for RpcStub and RpcPromise. They just return `[object RpcStub]` and `[object RpcPromise]`, but that's better than the previous `[object Function]` which was confusing. As suggested in #55. * Fix type signature of newHttpBatchRpcSession(). An earlier version of the function was designed to have the same signature as `fetch()`, but when I added `RpcSessionOptions` it made more sense to make that the second param... but I only updated the implementation and forgot to update the type. Fixes #67. * Support serializing Infinity, -Infinity, and NaN. Fixes #80 * Polyfill Promise.withResolvers(). This hopefully improves compatibility with old Safari versions and Hermes (React Native). Unfortunately it didn't seem easy to extend the tests to cover React Native. There is a vitest-react-native package, but it looks little-used and unmaintained, so I didn't want to install it. Might help with #91, though I won't declare that one "fixed" until we actually have tests proving it. * Document missing expression types in protocol.md. Fixes #48
Fixes #23
Per #23, many bundlers do not like our dynamic import of "cloudflare:workers". This PR takes a different approach, using the "exports" feature of "package.json" to point workerd at a different version of the code. That version imports "cloudflare:workers" and sticks it in the global scope before importing the rest of the library, thus allowing it to conditionally probe.
@threepointone is this a sane approach? I have very little understanding of JS tooling but this seem like it works.