pmndrs / use-cannon

👋💣 physics based hooks for @react-three/fiber
https://cannon.pmnd.rs
2.76k stars 154 forks source link

[types] mutableRefObject should default to null #242

Closed bjornstar closed 3 years ago

bjornstar commented 3 years ago

The recommended use case for useRef is to use null as the default value. This should make refs more consistent and easier to reason about.

drcmda commented 3 years ago

is this a breaking change though? previously i can expect an object3d, now i have to check for null? also, is it really that common to do MutableRef<Object3D | null> instead of just MutableRef<Object3D> + useRef<Object3D>(null!) (@joshuaellis showed me that)

i've been using the latter w/o problems. what was the issue you were facing?

bjornstar commented 3 years ago

I don't believe this is a breaking change.

The problem is that the default value for current is null, not undefined:

declare namespace React {
    interface RefObject<T> {
        readonly current: T | null;
    }

    function createRef<T>(): RefObject<T>;

We use createRef<Object3D>() in our store outside of the hook lifecycle to share the reference between components.

Using MutableRefObject<Object3D | null> everyone can continue using useRef<Object3D>(null!), but it opens up the ability for us to use createRef<Object3D>(). The only thing people would have to change is useRef<Object3D>() which no one should be using.

bjornstar commented 3 years ago

I started converting the examples to typescript and immediately ran into this problem. Any interest in merging this?

joshuaellis commented 3 years ago

I can see why the props would be useful to have null added so people can do useRef<Object3D>(null) but the localRefs where we know what it's going to be etc. we could keep the non-null assertion no?

EDIT: I don't know much about this library, so just offering a TSxReact opinion

bjornstar commented 3 years ago

I've gone ahead and updated the readme. If there are no objections, I'd like to merge this.

bjornstar commented 3 years ago

There are no functional changes here, but if you're using typescript and defining some of the refs as undefined they'll need to be changed to null instead. It's now consistent and we can use createRef without trouble.