-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
refactor(core): expose core without react as entrypoint #2466
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 2f9f379:
|
@@ -33,7 +31,6 @@ export interface IntersectionEvent<TSourceEvent> extends Intersection { | |||
stopped: boolean | |||
} | |||
|
|||
export type Camera = THREE.OrthographicCamera | THREE.PerspectiveCamera |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was duplicated in utils.ts
so just imported from Line 2
// https://github.com/facebook/react/tree/main/packages/react-reconciler#getcurrenteventpriority | ||
// Gives React a clue as to how import the current interaction is | ||
export function getEventPriority() { | ||
let name = window?.event?.type | ||
switch (name) { | ||
case 'click': | ||
case 'contextmenu': | ||
case 'dblclick': | ||
case 'pointercancel': | ||
case 'pointerdown': | ||
case 'pointerup': | ||
return DiscreteEventPriority | ||
case 'pointermove': | ||
case 'pointerout': | ||
case 'pointerover': | ||
case 'pointerenter': | ||
case 'pointerleave': | ||
case 'wheel': | ||
return ContinuousEventPriority | ||
default: | ||
return DefaultEventPriority | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to react/utils.ts
@@ -1,6 +1,5 @@ | |||
import * as THREE from 'three' | |||
import * as React from 'react' | |||
import create, { GetState, SetState, StoreApi, UseBoundStore } from 'zustand' | |||
import create, { GetState, SetState, StoreApi } from 'zustand/vanilla' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using vanilla store for core
and is consumed with create
from zustand
in react/index.tsx
::createRoot
export interface Intersection extends THREE.Intersection { | ||
eventObject: THREE.Object3D | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated in core/events.ts
where it made more sense, so imported from there
/** | ||
* An SSR-friendly useLayoutEffect. | ||
* | ||
* React currently throws a warning when using useLayoutEffect on the server. | ||
* To get around it, we can conditionally useEffect on the server (no-op) and | ||
* useLayoutEffect elsewhere. | ||
* | ||
* @see https://github.com/facebook/react/issues/14927 | ||
*/ | ||
export const useIsomorphicLayoutEffect = | ||
typeof window !== 'undefined' && (window.document?.createElement || window.navigator?.product === 'ReactNative') | ||
? React.useLayoutEffect | ||
: React.useEffect | ||
|
||
export function useMutableCallback<T>(fn: T) { | ||
const ref = React.useRef<T>(fn) | ||
useIsomorphicLayoutEffect(() => void (ref.current = fn), [fn]) | ||
return ref | ||
} | ||
|
||
export type SetBlock = false | Promise<null> | null | ||
export type UnblockProps = { set: React.Dispatch<React.SetStateAction<SetBlock>>; children: React.ReactNode } | ||
|
||
export function Block({ set }: Omit<UnblockProps, 'children'>) { | ||
useIsomorphicLayoutEffect(() => { | ||
set(new Promise(() => null)) | ||
return () => set(false) | ||
}, [set]) | ||
return null | ||
} | ||
|
||
export class ErrorBoundary extends React.Component< | ||
{ set: React.Dispatch<Error | undefined>; children: React.ReactNode }, | ||
{ error: boolean } | ||
> { | ||
state = { error: false } | ||
static getDerivedStateFromError = () => ({ error: true }) | ||
componentDidCatch(err: Error) { | ||
this.props.set(err) | ||
} | ||
render() { | ||
return this.state.error ? null : this.props.children | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to react/utils.ts
import { | ||
appendChild, | ||
createInstance, | ||
insertBefore, | ||
Instance, | ||
InstanceProps, | ||
LocalState, | ||
prepare, | ||
removeChild, | ||
Root, | ||
} from '../core/renderer' | ||
import { attach, detach, diffProps, DiffSet, invalidateInstance, is, applyProps } from '../core/utils' | ||
import Reconciler from 'react-reconciler' | ||
import { UseBoundStore } from 'zustand' | ||
import { RootState } from '../core/store' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reconciler was the React-specific part of the createRenderer, splitting it into two parts, where the core exposes a renderer and the ReactReconciler operates using the renderer.
This is very unsafe to do, R3F's renderer internals assumes it's working in a React context and implementing the Fiber reconciler architecture. Its internal interfaces may change at any time and are not subject to semver. Exposing them is a non-starter. However, events are indeed framework agnostic and are implemented entirely using public methods. They are an optional plugin to R3F and can be split from the core without too much trouble. There has been talk in the past with angular-three in breaking that bit up and sharing that which I'd encourage on the topic of re-use. |
Even without exposing, I think the code organized like this makes it way easier for a fork to be maintained to support other frameworks. Right now the |
We're not able to support this, and I don't believe it's in the best interest of either project if we were to. Implementations can and will grow divergent, especially as you lean into Solid's features and as R3F's architecture changes to suit its interests (e.g. #2250, https://github.com/pmndrs/react-ogl). I've outlined what is needed to make this work in https://github.com/CodyJasonBennett/solid-three-renderer. It's not a lot of code, yet it's much more robust than R3F atm, especially regarding type-safety, missing reconstruct logic and the hook bits. |
Okay that does make kinda sense and you guys have spent way more time here so I'm definitely missing something, but I found the core you guys built with a render loop, events, attach/detach, prop-application, all of it really helpful and kinda independent of React except maybe the memoization which might be helpful anyway. The maintenance burden and potential slowdown in improvement is definitely a concern. The custom renderer you implemented is quite helpful, I already had that implemented in solid-three which is was a port of all the functionality from r3f. I still would like something like that, so going to keep maintaining my fork as the official |
The main thing I was seeking was a good way to be able to stay with the changes you guys make that would be helpful to solid-three users. I think the rebase is not that bad so I can continue to do that until its unfeasible. |
If we're going to share code, we should do so in a mutual way that is async from the release cycle of either project -- a separate package from R3F, as I hinted with events in #2466 (comment). I don't believe this would include loop code or diffing/prop logic as they're performance sensitive and dig into framework internals (referring to both react and R3F). Consequently, I think we should move discussion to a separate issue as this is not yet actionable via PR. |
I agree with @CodyJasonBennett on this one. I don't think this is a good direction for the library and if we were to expose these things for other libs we then hold a maintenance burden for the libraries that use said code which therefore could potentially stunt the growth of this library. |
Refactors
core
to make it framework-agnostic, and expose it at@react-three/fiber/core
for other frameworks to build integrations on.Motivation:
I think the work being done here is extremely valuable to pushing the ball forward on 3D on the web. I think there is scope for much broader usage if other frameworks can build integrations on top of the core in r3f. I was working on solid-three and was able to use 90% of the code from the codebase here.
I understand that the team doesn't want to burden itself with maintaining integrations with other frameworks. I think a small step can be taken which would be to expose a React-free core at
@react-three/fiber/core
or possible@react-three/core
(not necessary). This will make the Three world easier to consume for other frameworks too. I know there is still a lot of work on top of that to get to parity, but its a start I think that I know immediately atleast Solid and Vue should be able to useAll the reconciler and hook-related stuff can be isolated. The most valuable stuff is the dom-like abstraction, so
createInstance
,insertBefore
,appendChild
, etc., and the events system. All of this stuff was react-free anyway. For now, I split thecore
intocore
andreact
and everything remains the same with just some changed imports.Any questions or thoughts about this, I am free to chat.
And thanks for the awesome work ❤️