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(types): canvas type improvements #2422

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

kmannislands
Copy link
Contributor

@kmannislands kmannislands commented Aug 4, 2022

Improve typing on createRoot by declaring that it handles HTMLCanvasElement | OffscreenCanvas. This is more expressive than accepting any subclass of Element and allows some generic type params to be removed.

Two reasons this is a draft:

  • Though removing the generic type param from createRoot seems like a clear DX improvement, the function is part of the publicly documented API of r3f so this could be considered a small breaking change
  • To discuss approaches on how to source the OffscreenCanvas typing. Currently, I import it from the three definitely typed definitions but the def there seems non-ideal. I was a bit surprised that the type isn't defined in dom.lib.d.ts. Poking around, it seems that it is, however, defined in lib.webworker.d.ts meaning that it's possible to modify the tsconfig to include WebWorker. I wouldn't recommend this approach since generally the code in this repo is intended for the main thread so making worker-side types universally available is probably a bad idea. Open to suggestions here.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 4, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 056492f:

Sandbox Source
example Configuration

@kmannislands
Copy link
Contributor Author

Keen to hear your thoughts on typing OffscreenCanvas @joshuaellis (thanks for taking over the three.js typings btw)

@CodyJasonBennett CodyJasonBennett marked this pull request as draft August 5, 2022 22:48
@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Sep 15, 2022

I'm looking to pick this PR into v9 (#2331), although the only withstanding issue would be with the question of how to source OffscreenCanvas. I'm very wary of relying on types' dependencies as it undermines our ability to follow semver, and we've already been bitten by this with WebXR which necessitates the major bump in v9.

@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review February 28, 2023 05:15
@CodyJasonBennett CodyJasonBennett changed the title Draft: Canvas type improvements fix(types): canvas type improvements Feb 28, 2023
@CodyJasonBennett
Copy link
Member

I coincidentally mirrored this in #2770 and #2774, so merging as complete. I'll relegate removing the redundant generics from RenderProps etc. to v9.

@CodyJasonBennett CodyJasonBennett merged commit c188c5d into pmndrs:master Feb 28, 2023
itsdouges pushed a commit to itsdouges/react-three-fiber that referenced this pull request Jan 2, 2024
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.

2 participants