firtoz / react-three-renderer-fiber

Porting R3R to use React Fiber
MIT License
94 stars 10 forks source link

Refactor browser logic to more universal approach #54

Open birkir opened 6 years ago

birkir commented 6 years ago

Hey toxicFork. Great work man!

One idea... Currently there are a lot of references to browser api methods in modules such as DevtoolsHelpers and webglRenderer. Refs and even requirements to things like console, window, document etc.

Also the Reconciler has a hard requirement of ReactDOM. There must be a better way of initializing the react context better without the fake dom node.

If this would be possible we could make r3r-fiber work with webgl ports in other platforms, such as react-native with react-native-webgl.

Currently working on refactoring the context construction with the recently released React.createContext api, but any ideas or i told you so's appreciated.

toxicFork commented 6 years ago

First of all, good idea!

Yes, currently the renderer is still very much tied to threejs' browser implementation, but it should not be too difficult to identify parts of it that can be disconnected and refactored, or modularised. For example we could have a core module and a "threejs-browser" module and "threejs-reactnative" module. Additionally things like dev tools and console can be made to be turned on/off or mocked/replaced.

React.createContext api

The name of this excites me! Can you please link to relevant documentation or resources you have found? It will help :)

On Thu, Feb 8, 2018, 00:57 Birkir Rafn Guðjónsson notifications@github.com wrote:

Hey toxicFork. Great work man!

One idea... Currently there are a lot of references to browser api methods in modules such as DevtoolsHelpers and webglRenderer. Refs and even requirements to things like console, window, document etc.

Also the Reconciler has a hard requirement of ReactDOM. There must be a better way of initializing the react context better without the fake dom node.

If this would be possible we could make r3r-fiber work with webgl ports in other platforms, such as react-native with react-native-webgl.

Currently working on refactoring the context construction with the recently released context api.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/toxicFork/react-three-renderer-fiber/issues/54, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0iLe2l_ZzlouVbwR45x5GY229yBZshks5tSkZ4gaJpZM4R9ouj .

birkir commented 6 years ago

Awesome! Here is a blog post taking on the new (undocumented) createContext api.

https://blog.kentcdodds.com/reacts-%EF%B8%8F-new-context-api-70c9fe01596b

birkir commented 6 years ago

Here are the changes I had to make to react-three-renderer-fiber just to make it compile/bundle for react-native.

https://gist.github.com/birkir/c86b315df390fe485b76d96c0ffe4545

Here is a repo with it fully working, but it won't react to state changes because there isn't any context.

https://github.com/birkir/react-native-three-renderer/commit/16ad208fe0f6ed8416b78d50392c388a0bf88106

I am too unfamiliar with the codebase to create quality refactor PR so bear with me while I'm digging 😇

toxicFork commented 6 years ago

This is very cool, great work! :D Also thanks for the createContext blog post link!

On 8 February 2018 at 18:45, Birkir Rafn Guðjónsson < notifications@github.com> wrote:

Here are the changes I had to make to react-three-renderer-fiber just to make it compile/bundle for react-native.

https://gist.github.com/birkir/c86b315df390fe485b76d96c0ffe4545

Here is a repo with it fully working, but it won't react to state changes because there isn't any context.

https://github.com/birkir/react-native-three-renderer

I am too unfamiliar with the codebase to create quality refactor PR so bear with me while I'm digging 😇

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/toxicFork/react-three-renderer-fiber/issues/54#issuecomment-364209501, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0iLYqSowxXAtWh-TikGRegNsqv7fGiks5tS0DIgaJpZM4R9ouj .