-
Notifications
You must be signed in to change notification settings - Fork 156
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
Create API layer between hooks and cannon web worker #343
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/use-cannon/Ev4hdUedpSPXZfunE2P2YP3FfrZg |
7278af0
to
7ff7efc
Compare
src/worker/cannon-worker.ts
Outdated
CannonWebWorker, | ||
DefaultContactMaterial, | ||
DisableConstraintMessage, | ||
DisableConstraintMotorMessage, |
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.
I feel like we could have a single type that we import instead of each individually named message.
I'd like to remove the need for WithoutOp as well.
Maybe we can do something like:
CannonMessageBody<'disableConstraintMotor'>
or
CannonMessageMap['disableConstraintMotor']
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.
Sure! I can get rid of all the imports and WithoutOp
by putting an omit in something like CannonMessageBody
, sounds good.
I'll update the PR with this & the rebase shortly. Should have time to do this tomorrow 🙂
I'm actually ok with everything and moving forward from here. Does it seem like a usable api for the worker? |
Great! Thanks for taking a look. I think the worker API as it is now is largely good for building out the non-react package. There are a couple of things we may want to add/change as we go. One thing that comes to mind is - if we're building the other package to mirror cannon-es syntax/usage as we discussed in the issue, we may add But it's probably best to look at making those changes in small chunks as we build out that package. For now, wrapping the current worker as-is sounds good to me 🙂 I'll update the PR to address your comments & rebase shortly. |
7ff7efc
to
50b348e
Compare
50b348e
to
732ad98
Compare
732ad98
to
4ce8d3b
Compare
Motivation: This is a step towards moving worker logic into a separate package
4ce8d3b
to
1042071
Compare
@bjornstar I've rebased now & addressed your comments! |
CannonWorker
between hooks and cannon web workerOperation
type to support props that are undefinedtypescript-eslint/member-ordering
for better ordering for classes