Skip to content

fix: createScope root param type for ReactRef #971

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

Conversation

Jimmydalecleveland
Copy link
Contributor

@Jimmydalecleveland Jimmydalecleveland commented Apr 5, 2025

First, this is such an amazing library, thank you to everyone who has contributed to making it so lovely.

Issue

The createScope method accepts ScopeParams which allows the root property to be ReactRef as part of the union and that type does not accept null.

The React libraries type definition for refs is currently: type Ref<T> = RefCallback<T> | RefObject<T | null> | null;

null is the valid state for React refs attached to DOM nodes during their initial render or when unmounting, so if createScope should allow passing the ref directly, as currently shown in the documentation, null should be allowed.

For example (from the current docs):

// ...
const scope = useRef(null);

useEffect(() => {
  scope.current = createScope({ root })
// ...

return (
  <div ref={root}>
//...

This would currently have a typescript error for root: Type 'null' is not assignable to type 'HTMLElement | SVGElement | undefined'


I did not encounter any type errors that did not already exist (on "master" there is currently an AbortSignal error), and since the Scope class does a truthy check (if (rootParam) { //... }) I believe this change shouldn't cause any issues since undefined is already accounted for and not explicitly checked.

@juliangarnier
Copy link
Owner

juliangarnier commented Apr 5, 2025

Hey thanks for the PR!
I have to write the contribution guide since this is clearly not obvious, but the types are generated from the JSDoc annotations.
Basically, you have to define the type straight into the JS using JSDoc annotation, then run npm run dev-types and this will generates all the types from the JSDoc comments automatically in the index.d.ts file.

@Jimmydalecleveland
Copy link
Contributor Author

Hey thanks for the PR! I have to write the contribution guide since this is clearly not obvious, but the types are generated from the JSDoc annotations. Basically, you have to define the type straight into the JS using JSDoc annotation, then run npm run dev-types and this will generates all the types from the JSDoc comments automatically in the index.d.ts file.

Oh, woopsie. Thank you for letting me know.

@Jimmydalecleveland Jimmydalecleveland force-pushed the fix/create-scope-root-type-for-react-ref branch from 7da1f9d to f0af3c1 Compare April 7, 2025 19:23
The `createScope` method accepts `ScopeParams` which allows the `root` property to be
`ReactRef` as part of the union and that type does not accept `null`.

The React libraries type definition for refs is currently:
`type Ref<T> = RefCallback<T> | RefObject<T | null> | null;`

`null` is the valid state for React refs attached to DOM nodes during their initial
render or when unmounting, so if `createScope` should allow passing the ref directly,
as currently shown in the documentation.

For example (from the current docs):
```jsx
// ...
const scope = useRef(null);

useEffect(() => {
  scope.current = createScope({ root })
// ...

return (
  <div ref={root}>
//...
```

This would currently have a typescript error for `root`:
`Type 'null' is not assignable to type 'HTMLElement | SVGElement | undefined'`
@Jimmydalecleveland Jimmydalecleveland force-pushed the fix/create-scope-root-type-for-react-ref branch from f0af3c1 to bee26c8 Compare April 7, 2025 19:28
@Jimmydalecleveland
Copy link
Contributor Author

Ok, hopefully that update applies the fix properly.

side note: I had some trouble getting the npm run dev-types command to run on node LTS, and had to go back to v20 and apply some fixes to the rollup config, which I of course didn't include. There isn't an engines property in the package.json for me to reference what version this is intended to run on, and I didn't see anything in the README so forgive me if I'm missing something obvious, but that may be inhibiting other contributors. Just a heads up in case you aren't already aware.

@juliangarnier juliangarnier merged commit d29fa22 into juliangarnier:master Apr 9, 2025
@juliangarnier
Copy link
Owner

Ok, hopefully that update applies the fix properly.

Thanks, I just merged and published 4.0.1 with you fix.

side note: I had some trouble getting the npm run dev-types command to run on node LTS, and had to go back to v20 and apply some fixes to the rollup config, which I of course didn't include. There isn't an engines property in the package.json for me to reference what version this is intended to run on, and I didn't see anything in the README so forgive me if I'm missing something obvious, but that may be inhibiting other contributors. Just a heads up in case you aren't already aware.

OK, I think it comes from the fact that I'm using an outdated version of Rollup in order to use the plugin rollup-plugin-ts.
I couldn't figure out how to get the same workflow regarding the types generation from JSDoc with @rollup/plugin-typescript and rollup-plugin-dts, and didn't want to spend too much time on this...

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