Fix: initial canvas size computation in createRoot #2406
Conversation
|
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 596a67d:
|
| @@ -3,12 +3,12 @@ import { Root } from './renderer' | |||
| import { RootState, Subscription } from './store' | |||
|
|
|||
| type GlobalRenderCallback = (timeStamp: number) => void | |||
| type SubItem = {callback: GlobalRenderCallback} | |||
| type SubItem = { callback: GlobalRenderCallback } | |||
There was a problem hiding this comment.
Unrelated formatting changes caused by running lint locally. Is eslint not checked in CI?
There was a problem hiding this comment.
We have husky hooks for housekeeping on commit, but prettier won't fail a PR. Doesn't work when people edit directly in GitHub though.
There was a problem hiding this comment.
Would you be open to a PR that adds lint checking in CI?
There was a problem hiding this comment.
Great. Opened #2420
packages/fiber/src/core/index.tsx
Outdated
| function createRoot<TCanvas extends Element>(canvas: TCanvas): ReconcilerRoot<TCanvas> { | ||
| if (!elementIsCanvas(canvas)) { |
There was a problem hiding this comment.
I may be missing context here. The generic TCanvas seems strange since it seems like this function wouldn't work if canvas is an arbitrary element other than canvas.
There was a problem hiding this comment.
Indeed. Although I'd prefer to check with instanceof HTMLCanvasElement if it works for OffscreenCanvas.
There was a problem hiding this comment.
I've improved this to handle OffscreenCanvas explicitly and split this work out to hopefully allow the less controversial bug fix here to go ahead:
kmannislands#2
I'll re-target this PR to r3f if there's appetite for the change.
There was a problem hiding this comment.
I'd have to defer to @joshuaellis on how to type this in kmannislands#2. I'm a little wary of consuming types from @types/three rather than fixing upstream.
kmannislands
force-pushed
the
fix-size-computation
branch
from
b1f5930 to
596a67d
Compare
As @Madou noted here, the
clientTopused in the previous MR aroundSeems this would only effect users of direct
createRootand notCanvasbut still worth correcting.Did a small amount of boyscoutting to remove
lets and improve types here. The notable change is using a typeguard to throw at the top ofcreateRootif the supplied element is not a canvas. This allows cleaner typings after the guard. Specifically, the samecanvasvariable is later passed tocreateRendererInstancewhere it is cast toHTMLCanvasElementand I assume would fail if the cast is inaccurate. Happy to revert this part of the change if it is not desired.