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

Suggest fix for useRef-types in hooks #3222

Merged
merged 8 commits into from Nov 17, 2021
Merged

Suggest fix for useRef-types in hooks #3222

merged 8 commits into from Nov 17, 2021

Conversation

@JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Jul 8, 2021

No description provided.

@coveralls
Copy link

@coveralls coveralls commented Jul 8, 2021

Coverage Status

Coverage remained the same at 99.624% when pulling 29cacee on suggest-ref-fix into cc0f3fb on master.

@robertknight
Copy link
Member

@robertknight robertknight commented Jul 9, 2021

What is the rationale for these changes? Is there a related issue?

@tomasklaen
Copy link

@tomasklaen tomasklaen commented Jul 9, 2021

This originated from slack. The issue is that Ref types are broken since 10.5.14:

image

This is because components expect Ref<T | null>, but the useRef hook overload with no params is typed as:

declare function useRef<T>(): Ref<T | undefined>;

The quickest fix for this would be to just switch the undefined for null, which is how I'm currently patching the preact module types in my project.

But lets make this an opportunity to fix the whole preact Ref typing situation. As developit said it on slack, there are currently 4 or 6 different Ref types spread over preact and preact/hooks type definitions. I've personally suffered because of this when implementing interfaces that consume Ref objects.

The ideal situation would be to have just one set in stone Ref type with no side effects or magic:

type Ref<T> = {current: T};

And since it's implemented in core, put it into preact type definitions, and then just import it inside preact/hooks.

No special Refs with null or readonly. That just makes things more complicated, confusing, and harder to implement Ref consuming interfaces.

Also, although readonly would be useful for Ref objects intended to be passed into component's ref attribute, you just can't make this assumption beforehand, because we don't know where the Ref object that is being created is going to end up or how it's going to be used. This will lead to people confused by why they can't edit their Ref object, which should be mutable.

@JoviDeCroock
Copy link
Member Author

@JoviDeCroock JoviDeCroock commented Jul 9, 2021

@tomasklaen these are similar types to other frameworks implementing these paradigms (can be seen in React as well) I would stick to the deterministic nature of readonly for dom-nodes atleast as the assumption can be struck that we'll start out undefined and let an external actor take care of it.

@tomasklaen
Copy link

@tomasklaen tomasklaen commented Jul 9, 2021

I see that this is pretty much 1:1 copy of how react defines their types, and compatibility with react is definitely a good thing.

I've also realized you can make non initialized refs mutable by adding | null to T, which addresses my issues. So no more objections from me :)

But if we care about compatibilty with react, the interfaces should be RefObject and MutableRefObject. If we don't, they should be Ref and MutableRef. Right now they are a mix of both.

Oh and what about consolidating all refs from both preact's main definition file and hooks definitions into these 2, defined in core, and imported into hooks? Or do we not care that core has its own different Ref interfaces?

@JoviDeCroock JoviDeCroock requested a review from rschristian Nov 17, 2021
@JoviDeCroock JoviDeCroock merged commit d744e24 into master Nov 17, 2021
3 checks passed
@JoviDeCroock JoviDeCroock deleted the suggest-ref-fix branch Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants