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

BREAKING CHANGE: change accessing children from useInstanceHandle()'s… #302

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aryankarim
Copy link

I've manually copied and pasted the dist built by this branch into my NextJs v.15 which uses React v.19. Everything works.

However, I'm not sure about the semantic versioning.
Developers should install as npm install @react-three/[email protected] IMO.

This is my first every PR. Please let me know if I've missed anything.

Closes #301

@chrisweb
Copy link

chrisweb commented Nov 18, 2024

Thank you for working on this 👍

Regarding the versions of the packages, maybe it would be best to use the same values as does r3f v9.0.0-rc.1, this is what they use in their package.json:

    "react": "19.0.0-rc.1",
    "react-dom": "19.0.0-rc.1",
    "@types/react": "npm:types-react@beta",
    "@types/react-dom": "npm:types-react-dom@beta",

you could also update r3f in the @react-three/postprocessing package.json to this:

"@react-three/fiber": "9.0.0-rc.1",

and then your fix could for example go into a @react-three/postprocessing 3.0.0-rc.0 (or skip the rc0 and start with rc.1 to align with react and r3f!?) and get tagged with rc (same as r3f did)

EDIT: @aryankarim what version of r3f did you use? Because I tried out your fix, but the types you import don't seem to match what gets exported from r3f v9.0.0-rc.1 (or rc.0), but other than that your fix seems to work :) (in my test I just undid the changes lines 12 and 25 that are related to types and kept the rest)

@aryankarim
Copy link
Author

aryankarim commented Nov 18, 2024

   "@react-three/postprocessing": "^2.16.3",
    "@react-three/fiber": "^9.0.0-alpha.8",
    "@types/three": "^0.169.0",
    "next": "15.0.2",
    "react": "19.0.0-rc-02c0e824-20241028",
    "react-dom": "19.0.0-rc-02c0e824-20241028",

I used r3f v9.0.0-alpha.8 for my testing environment (I don't remember how I got that version, tbh).

Well with r3f 9.0.0-rc.1, I get two other TS compilation errors on build:

src/Selection.tsx:9:47 - error TS2339: Property 'group' does not exist on type 'IntrinsicElements'.

9 export type SelectApi = JSX.IntrinsicElements['group'] & {
                                                ~~~~~~~

src/util.tsx:12:72 - error TS2694: Namespace '"react-postprocessing/node_modules/@react-three/fiber/dist/declarations/src/three-types"' has no exported member 'Node'.

12 export type EffectProps<T extends EffectConstructor> = ReactThreeFiber.Node<

Let me see if I'm gonna be able to fix it with rc.1 and without adding those types.

@chrisweb
Copy link

chrisweb commented Nov 19, 2024

@aryankarim actually the JSX.IntrinsicElements['group'] type problem (I bumped into it last week when working on the animation for my blog), I added JSX to the React imports and this solved it, so something like this:

export type SelectApi = React.JSX.IntrinsicElements['group'] & {

or just add it to the import itself like this:

import React, { createContext, useState, useContext, useEffect, useRef, useMemo, type JSX } from 'react'

@aryankarim
Copy link
Author

Yep, that solved the first one. I also noticed the new r3f uses ThreeElement interface to declare new nodes, so I think I solved the second problem too.

@chrisweb Could you try this new commit, see if it works with your environment?

Also, we can get @CodyJasonBennett or @abernier 's attention, to know if these changes are enough for making postprocessing work with latest r3f and react?!?

@chrisweb
Copy link

chrisweb commented Nov 21, 2024

@aryankarim works great 👍 I had a small problem I will describe below, but other than that the build process had no errors and then I used it in the repo in which I found the problem and can confirm that it is solved

So I just had one problem in the utils.tsx file, my imports now look like this:

- import { Vector2 } from 'three'
- import React, { MutableRefObject } from 'react'
+ import React from 'react'
import * as THREE from 'three'
- import { type ReactThreeFiber, ThreeElement, extend, useThree } from '@react-three/fiber'
+ import { type ReactThreeFiber, type ThreeElement, extend, useThree } from '@react-three/fiber'
import type { Effect, BlendFunction } from 'postprocessing'

I had to add type in front of ThreeElement, without it I would get an ESLint error:
ThreeElement not found in '@react-three/fiber' eslint(import/named)

Oh yeah the other small difference is that I saw that everywhere in the code of the utils file the previous author(s) had used React.foo, Three.bar, ... which probably led to MutableRefObject and Vector2 to become unused, so I removed them too

@aryankarim
Copy link
Author

@chrisweb Lol seems like the type issues are persistent. I believe we need to go over each file and make sure everything is safely typed and is up-to-date with R3F and React 19. I think that they would do the same thing if they ever come back maintaining this code.

I'm afraid if I started making lots of changes, they wouldn't accept it or something.

package.json Outdated
Comment on lines 65 to 66
"@types/react": "^18.2.0",
"@types/react": "^18.3.12",
"@types/react-dom": "^18.2.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this should be:

"@types/react": "npm:[email protected]",
"@types/react-dom": "npm:[email protected]",

which may also need:

"overrides": {
  "@types/react": "npm:[email protected]",
  "@types/react-dom": "npm:[email protected]"
}

This is detailed in https://react.dev/blog/2024/04/25/react-19-upgrade-guide, and we have our own https://github.com/pmndrs/react-three-fiber/blob/v9/docs/tutorials/v9-migration-guide.mdx.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@CodyJasonBennett
Copy link
Member

Thanks for this. I've been busy with React 19 changes and conferences, but this is something we are interested in getting across the line as we're closer to release. Drei has been at the forefront of our concern, but my hope is this library will only need type changes, which can be checked off with the compiler/IDE. I'm about to get on another plane, but happy to test and answer any questions in the coming days.

@aryankarim
Copy link
Author

Great. Yes, I think Drei has more priority to function properly. Good luck with your trips and conferences.

Indeed, except on the EffectComposer's line 137, all the changes have been type related.

With my latest commit, I have tested a bunch of effects on a Nextjs 15 environment.
ToneMapping, Glitch, Noice, DotScreen All works fine.

I haven't tested AutoFocus, Outline yet. I can go over those and the others when I have free time and PR them if they didn't work.

@stijlfigurant
Copy link

Thanks for this contribution @aryankarim this has for now unblocked me trying to upgrade a project using r3f to next.js 15 / react 19

@teneburu
Copy link

teneburu commented Dec 6, 2024

Thank you @aryankarim, you're a savior !!

@mlenser
Copy link

mlenser commented Dec 13, 2024

@stijlfigurant @teneburu how are you guys getting this to work? I've tried

"@react-three/drei": "^9.120.4",
"@react-three/fiber": "^9.0.0-rc.1",
"@react-three/postprocessing": "github:aryankarim/react-postprocessing#9bd48e942aa9607295c3ccaed220d072bb352998",
"postprocessing": "^7.0.0-beta.4",
"three": "^0.171.0"

Locally it works fine. On Github actions when trying to install dependencies via pnpm it runs some yarn install, but fails on

Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/Bloom.tsx(4,14): error TS2742: The inferred type of 'Bloom' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/BrightnessContrast.tsx(4,14): error TS2742: The inferred type of 'BrightnessContrast' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/ChromaticAberration.tsx(5,14): error TS2742: The inferred type of 'ChromaticAberration' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/ColorDepth.tsx(4,14): error TS2742: The inferred type of 'ColorDepth' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/Depth.tsx(4,14): error TS2742: The inferred type of 'Depth' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/DotScreen.tsx(4,14): error TS2742: The inferred type of 'DotScreen' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/FXAA.tsx(4,14): error TS2742: The inferred type of 'FXAA' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/HueSaturation.tsx(4,14): error TS2742: The inferred type of 'HueSaturation' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/Noise.tsx(4,14): error TS2742: The inferred type of 'Noise' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/SMAA.tsx(4,14): error TS2742: The inferred type of 'SMAA' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/SSAO.tsx(8,14): error TS2742: The inferred type of 'SSAO' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/three'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/ScanlineEffect.tsx(4,14): error TS2742: The inferred type of 'Scanline' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/Sepia.tsx(4,14): error TS2742: The inferred type of 'Sepia' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/ShockWave.tsx(4,14): error TS2742: The inferred type of 'ShockWave' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/TiltShift.tsx(4,14): error TS2742: The inferred type of 'TiltShift' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/TiltShift2.tsx(90,14): error TS2742: The inferred type of 'TiltShift2' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/Vignette.tsx(4,14): error TS2742: The inferred type of 'Vignette' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
Error: ...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: src/effects/Water.tsx(31,14): error TS2742: The inferred type of 'WaterEffect' cannot be named without a reference to '.bin/store/v3/tmp/_tmp_1847_3fc05e84dfe9a05d62ec46be47c24d12/node_modules/@types/react'. This is likely not portable. A type annotation is necessary.
...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: error Command failed with exit code 1.
...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: error Command failed with exit code 1.
...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
...1847_3fc05e84dfe9a05d62ec46be47c24d12 yarn-install: Failed
 ERR_PNPM_PREPARE_PACKAGE  Failed to prepare git-hosted package fetched from "https://codeload.github.com/aryankarim/react-postprocessing/tar.gz/9bd48e942aa9607295c3ccaed220d072bb352998": @react-three/[email protected] yarn-install: `yarn install`
Exit status 1

@mlenser
Copy link

mlenser commented Dec 16, 2024

The above error seems to be caused by different versions of @types/react. I've forked the repo myself and tried to ensure it was always using "@types/react": "^19.0.1", but it still fails on CI (Linux). Somehow, somewhere a different version is sneaking in.

@CodyJasonBennett
Copy link
Member

See my review comment for Yarn usage. I noticed we needed overrides during testing in R3F.

@mlenser
Copy link

mlenser commented Dec 16, 2024

I have overrides at every level: my fork on this repo & the root level of my monorepo.

doesn't exist so I'm using

    "@types/react": "npm:@types/[email protected]",
    "@types/react-dom": "npm:@types/[email protected]",

This doesn't make a difference.

@EdamAme-x
Copy link

any news?

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.

r3f v9 (react 19) error in Effectcomposer
7 participants