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

Integrate brisk reconciler core #207

Merged
merged 28 commits into from
Jan 18, 2019

Conversation

bryphe
Copy link
Member

@bryphe bryphe commented Jan 14, 2019

WIP: Replace reason-reactify with brisk-core

This change:

  • Migrates our primitives to use the nativeComponent from brisk
  • Migrates our components to the brisk component model
  • Migrates our rendering logic to get the pending updates from brisk
  • Migrates example

Remaining work:

  • Finish Hello.re migration
  • Get fix for useState in nested tree ([Test case] State update in subtree not being picked up briskml/brisk#31)
  • Integrate useEffect implementation
  • Implement animation primitive w/ new useEffect
  • Hook up key handling in Calculator w/ useEffect
  • Find way to detect changes (so we can implement uiDirty primitive and only render when an update is needed)
  • Replace link: resolution with an NPM package for brisk-core
  • Update esy.lock w/ dependency update
  • Fix issue using Hooks.effect with condition OnMount

src/UI/UiRender.re Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
let subscribe = (evt: t('a), f: cb('a)) => {
evt := List.append(evt^, [f]);
let unsubscribe = () => {
evt := List.filter(f => f !== f, evt^);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to bring this to brisk-reconciler if possible. RemoteAction allows for just one subscriber.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wokalski , please feel free to bring it over 👍 (It had lived in reason-reactify before, anyway).

let text =
(
/* TODO: Proper way to downcast? */
let tn: TextNode.textNode = Obj.magic(node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Admittedly this is problematic about this API if you don't work with abstract types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. We'll likely revisit the decision to use class nodes or experiment / profile some alternate approaches.

@wokalski
Copy link
Collaborator

https://github.com/briskml/brisk-reconciler you can use this repo now!

@bryphe bryphe changed the title [WIP] Integrate brisk reconciler core Integrate brisk reconciler core Jan 18, 2019
@bryphe
Copy link
Member Author

bryphe commented Jan 18, 2019

Excellent, thanks @wokalski ! Just waiting on CI but I think this is almost ready 🎉

Are you planning on publishing an NPM package, too?

@bryphe bryphe merged commit e4d65a6 into master Jan 18, 2019
@bryphe bryphe deleted the bryphe/reconciler/integrate-brisk-reconciler branch January 18, 2019 03:02
@wokalski
Copy link
Collaborator

@bryphe if it’s not problematic for others I’d rather not just yet. I would like to rename a few types which might are technically breaking changes. Also, there might be further changes to the Hooks api (I.e. maybe you’ll return hooks from render). But when we have the ppx and the naming is ok, I’ll start publishing to npm.

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