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

Fiber #60

Merged
merged 18 commits into from
Dec 5, 2017
Merged

Fiber #60

merged 18 commits into from
Dec 5, 2017

Conversation

iamdustan
Copy link
Contributor

@iamdustan iamdustan commented Feb 28, 2017

Early Preview. Testing my approach from iamdustan/tiny-react-renderer#24.

Will update description later tonight or tomorrow.

To test:

  • Clone yarn install
  • node run.js
  • open second tab, run node_modules/.bin/react-devtools
  • enjoy.

This PR is intended to reimplement react-blessed on top of ReactFiberReconciler, the new (almost supported) renderer API. It is keeping the React 14.x stack reconciler support around for now by moving those files to src/stack. The shared utilities between stack and fiber have been moved to src/shared. The Fiber reconciler is entirely implemented right now in src/fiber/fiber.js. The other two files src/fiber/index.js and src/fiber/devtools.js exist to support react-devtools integration.

TODO:

Examples (as test cases)

  • render examples/animation.jsx
  • render examples/dashboard.jsx
  • render examples/demo.jsx

Reconciler API

  • removeChild
  • insertBefore
  • *Text* methods
  • clean up screen.render() calls
  • ensure event handlers work
  • consider using async scheduling (fiber feature)
  • consider supporting animation and deferred callbacks
  • figure out how to publish/version (current requirement involves npm installing an official renderer to get access to react-{renderer}/lib/ReactFiberReconciler.js

@iamdustan
Copy link
Contributor Author

Current status:

react-blessed-devtools

@iamdustan iamdustan changed the title WIP: On Fiber Initial implementation on Fiber Mar 1, 2017
@iamdustan iamdustan changed the title Initial implementation on Fiber Fiber Mar 1, 2017
@iamdustan
Copy link
Contributor Author

Note that 2447 lines of the diff is yarn.lock. The primary fiber implementation is 182 lines of code.

@Yomguithereal
Copy link
Owner

That's really cool @iamdustan. Tell me when you are done with it and will merge. I guess we'll have to wait a bit for react 16 to be released though.

figure out how to publish/version (current requirement involves npm installing an official renderer to get access to react-{renderer}/lib/ReactFiberReconciler.js

Should we engage a discussion with react team about this?

@iamdustan
Copy link
Contributor Author

I’ve half-spoken to them about how to distribute the reconciler. There isn’t a clear direction yet and I don’t think it’s high-pri. I’m going to start another conversation in the React Internals facebook group about it.

@iamdustan
Copy link
Contributor Author

react-blessed-events

Added a couple more examples and have event handlers working. Honestly, idk if the last three items (async work) matters at this point. ReactDOM has it disabled for the initial fiber release and I think that’s okay here, too.

Regarding the publishing stuff, I’ll be opening an official issue on the React issue tracker later tonight or tomorrow. I think merging and publishing this as an alpha is okay (it should be feature compatible with previous releases, just on fiber and requiring a specific package of react and react-dom for the time being)

@Yomguithereal
Copy link
Owner

Thanks @iamdustan. I'll try to review the code thoroughly soon. I agree the most blocking point at that time is the fact that we'll need to rely on another renderer to get the necessary files to depend on. Just ping me on the issue you'll open please.

@Yomguithereal
Copy link
Owner

I am also open to modify some features of the library to "modernize" them in the process. The class system comes to mind :)

@iamdustan
Copy link
Contributor Author

Issue for ReactFiberReconciler package opened at facebook/react#9103.
Re: modernizing: I’ve started looking at integrating flexbox layout via Yoga. :)

@Yomguithereal
Copy link
Owner

Yes there was some idea once to adapt the CSS layout to the blessed styles. But I am not sure to see what it would entail.

@sergey-lapin
Copy link

sergey-lapin commented Mar 4, 2017

Awesome work @iamdustan ! I have played with your example, but failed with making inputs work having this alt tag. Tried on iterm2 and terminal. Is there a way to set focus on input without mouse involved? this is 90% use case for cli apps, I believe

@Yomguithereal
Copy link
Owner

@Lapanoid you should check blessed's documentation about focus and inputs.

@iamdustan
Copy link
Contributor Author

@Lapanoid yeah, I’ve never had a lot of fun with blessed forms. I’ve had success with iterm2, but not with default Terminal.app

@Yomguithereal
Copy link
Owner

If I remember correctly, Terminal.app indeed has a lot of quirks, notably the mouse does not work and some focus events also. But it should be possible to devise a kind of form using tabulations & keys I guess.

@sergey-lapin
Copy link

@iamdustan My concern about react in terminal, that it should not break basic terminal functionality which are REPL and input.. All this gorgeous things will not get much traction if they are not play well with input.

@sergey-lapin
Copy link

@Yomguithereal sorry this is not right place for this discussion, I will create separate issue

@mashaal
Copy link

mashaal commented Mar 10, 2017

I had no luck using a list element -- errors out with TypeError: Cannot read property 'length' of undefined. Seems to work fine in previous version.

Edit: Upon further investigation, a list component will render correctly if it is the only element being rendered. Nesting inside another element will not.

@iamdustan
Copy link
Contributor Author

@mashaal could you post a brief demo and I’ll debug?

@mashaal
Copy link

mashaal commented Mar 10, 2017

@iamdustan https://gist.github.com/mashaal/2212d99f35dcadac2dfa096f7ae8623e

first example does render the list, second does not -- tried wrapping it in element, box, etc

@Yomguithereal
Copy link
Owner

@iamdustan don't hesitate to comment on the thread when you feel we can start to work towards a merge.

@skellock
Copy link

skellock commented Nov 6, 2017

This PR makes me happy in a way no PR should.

@iamdustan
Copy link
Contributor Author

Will do @Yomguithereal. 😄

Working through a few more issues (eg. facebook/react#11463) and then having you do an actual integration test somewhere would be really helpful.

@Yomguithereal
Copy link
Owner

an actual integration test

Do you have an idea on how to do that with such a library?

@iamdustan
Copy link
Contributor Author

Maybe I meant a manual test 😇

I’ve added a few new examples, but I’m not sure if I broke the dashboard one or if it never really worked for me.

@iamdustan
Copy link
Contributor Author

blessed

@Yomguithereal I think this is ready for a test drive.

@Delapouite
Copy link

Thanks ! I'll try to migrate https://github.com/byteclubfr/socket.io-monitor-cli over this new version to see how it goes.

package.json Outdated
},
"dependencies": {
"invariant": "^2.2.0",
"lodash": "^3.x.x"
"lodash": "^3.x.x",
Copy link
Owner

Choose a reason for hiding this comment

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

I guess we can upgrade lodash to v4 now.

package.json Outdated
"react-motion": "^0.5.2",
"react-reconciler": "0.5.0",
"ws": "^1.1.0",
"yoga-layout": "^1.2.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't those dependencies be in devDependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp. Yoga-layout was added as part of an unfinished experiment so I just removed it. I’m not really sure if ws should be a devDep, peerDep, or regularDep. It’s required to use react-devtools which can be used in production mode (I’m pretty sure on that). Putting that as a devDep means consumers would have to add ws on their side, right?

@Yomguithereal
Copy link
Owner

@iamdustan where are we on this? Doesn't some deps remain where they should not?

@iamdustan
Copy link
Contributor Author

I think that this is ready. I removed one of the dependencies completely, but I think the other may be necessary still for react-devtools to work.

@Yomguithereal
Copy link
Owner

This means devtool integration wouldn't work without those deps? Do you think we could offload some of them in peer deps with docs for people wanting to use the devtools.

@jonhermansen
Copy link

Is this PR ready to be used by others? Are there any remaining tasks that I can help with to push this along?

@Yomguithereal
Copy link
Owner

@jonhermansen. This PR should be usable right now. I am just waiting an answer regarding dependencies to merge.

@iamdustan
Copy link
Contributor Author

Derp. Missed the last question, @Yomguithereal. I just bumped a few versions and removed the dependency and added a guard during setup for it.

@Yomguithereal
Copy link
Owner

@iamdustan. I will merge & release now so that people can use your goodies. I'll try to find some time in the near future to work a bit on the library. Do you want me to add you as a contributor so you can freely edit the library?

@Yomguithereal Yomguithereal merged commit 371711b into Yomguithereal:master Dec 5, 2017
@Yomguithereal
Copy link
Owner

@iamdustan:

figure out how to publish/version (current requirement involves npm installing an official renderer to get access to react-{renderer}/lib/ReactFiberReconciler.js

Is this still the case?

@iamdustan iamdustan deleted the fiber branch December 11, 2017 18:26
@iamdustan
Copy link
Contributor Author

Negative, the official renderer means that is no longer valid.

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.

9 participants