refactor!: update to latest brisk-reconciler #576

lessp · 2019-10-01T19:54:25Z

~~So, pushing what I've got, I began converting the examples. Most of the should be OK now hopefully. Got a bit stuck on the UI_Primitives type-errors.~~

This PR introduces the changes from brisk-reconciler with the new syntax:

e.g.

let%component make = () => {
  let%hook (counter, setCount) = React.Hooks.state(0);

  <Clickable onClick={_ => setCount(prevCount => prevCount + 1)}>
    <Text style=Style.[...] text={string_of_int(counter)} />
  </Clickable>
};

lessp · 2019-10-03T13:19:18Z

Btw, this is a PR for merge into: #566

glennsl · 2019-10-28T19:43:29Z

examples/DefaultButton.re

@@ -5,7 +5,7 @@ open Revery.UI.Components;
 module DefaultButtonWithCounter = {
  let%component make = () => {
    let%hook (count, setCount) = Hooks.state(0);
-    let increment = () => setCount(count + 1);
+    let increment = () => setCount(_ => count + 1);


You're supposed to do setCount(count => count + 1); here. That's why it was made a function ;) It's unlikely to cause a problem if you don't, especially in this case where it worked absolutely fine before, but when it does cause a problem it's a tricky one to find because it requires breaking the abstraction. It also might be very hard to reproduce. See briskml/brisk-reconciler#26 for details.

Yep! I just mangled through all these and lazily replaced them with _!

EDIT: Could probably do a search and replace for (_ => and walk through them! 🙂

lessp · 2019-10-28T19:46:00Z

Ok, so with a few files commented out:

This builds and runs, with the core changes happening in UI/Reconciler.re and UI/Container.re

Et7f3 · 2019-10-29T12:25:16Z

I don't why I have a empty windows when executing esy run

lessp · 2019-10-29T13:48:36Z

~~Merged with master, some examples doesn't run:~~

Input~

Et7f3 · 2019-10-29T14:42:36Z

You should better rebase onto master instead of merging (you will avoid this kind of merge error).

js.json

src/UI/Container.re

src/UI_Components/Button.rei

src/UI_Components/Clickable.re

src/UI_Components/ExpandContainer.re

src/UI_Hooks/Animation.rei

test/UI/ReconcilerTests.re

Et7f3 · 2019-11-03T18:49:56Z

src/UI_Primitives/Image.re

+                      ~height=?,
+                      ~src="",
+                      ~style=Style.emptyImageStyle,
+                      ~children=React.empty,


Image can have children 👀

Same for other Primitives like OpenGL

I don't think this changes that, we just need t provide an empty children as a default which is needed IIRC!

Scrollview, Row, Container, Center must have one argument so default arg to React.empty is not fine for me. Ticker shouldn't have child.

I though not exposing argument and defining a local children variable. What do you think ? I can fix that.

That might make sense, but it differs from how master looks currently: https://github.com/revery-ui/revery/blob/master/src/UI_Components/Input.re#L356

I've updated all containers except Container as to explicitly take children @Et7f3.
Do you want me to push these, or do you want to create a separate issue for that?

Ah I see so create another issue. IIRC @bryphe wanted some compile time assertion to be verified.

for reference #489 (comment)_

lessp · 2019-11-03T18:55:36Z

Wonder if we should change the merge target to master instead of #566?

Et7f3 · 2019-11-03T19:24:56Z

When reviewing it I compared this branch and master and it was clean. I think it is better to change it.

bryphe · 2019-11-08T19:39:41Z

So cool to see this green and the let%hooks land! 🎉 Thanks for all the work / help with this!

Pretty heroic and impressive change @lessp @wokalski @Et7f3

Wonder if we should change the merge target to master instead of #566?

Sounds good to me! We can just close #566 in lieu of this more complete change 😄

bryphe

So cool to see the let%hook and let%component PPXs in action to remove the boilerplate we have today,

wokalski · 2019-11-08T19:52:20Z

The only problem is that let%hook doesn't work in reis currently. And it's hard to make it work there.

lessp · 2019-11-09T18:07:14Z

The only problem is that let%hook doesn't work in reis currently. And it's hard to make it work there.

Would you like to get this in before merging, or should we add it at a later stage? @wokalski @Et7f3

wokalski · 2019-11-09T19:51:37Z

No, it’s very hard. There are lower hanging fruits with more ROI.

src/UI_Components/Positioned.re

bryphe · 2019-11-11T16:14:28Z

Awesome! Let's bring this in - very exciting! 🎉 Thanks @wokalski @lessp @Et7f3

bryphe and others added 6 commits September 28, 2019 10:48

Update to latest brisk-reconciler

c1f1093

Add override for simple-component-api branch

783e46b

Add reference to PPX

adec991

Convert UI_Primitives to Brisk component PPX

e5cdc2f

First pass at converting UI_Components to Brisk ppx

78cf54e

chore: convert examples and some components

6c1c0da

lessp added 9 commits October 27, 2019 21:14

chore: begin conversion to latest brisk-reconciler

8ad88c5

refactor: can't use %component for stateless components

63207ea

refactor: update examples

0b82605

refactor: just working through type-errors

18872e6

chore: use listToElement

433e9e4

refactor: dropdown-example setState takes callback

c943d58

fix: update missed hook-calls

eeb274b

chore: at least it builds

5c7a51b

chore: lock-files, formatting

cf179a8

glennsl reviewed Oct 28, 2019

View reviewed changes

Et7f3 added 3 commits October 29, 2019 12:37

Fix dropdown

1bacffe

Fix RadioButton

4137dba

Fix FontExample

075d67f

lessp added 2 commits October 29, 2019 14:21

chore: merge master locally

f7e1399

refactor: fixes and updates after merge

3f7b937

lessp added 3 commits October 29, 2019 15:04

chore(Input): fix unused variable-warning

e26ad43

chore(Input): it was a duplicate, hence the warning..

afab3be

chore: fix warning in OpenGLExample

b28ec28

docs: animation-example fix after merge

552854a

Et7f3 reviewed Nov 3, 2019

View reviewed changes

lessp added 5 commits November 3, 2019 18:51

chore: get tests running again

245f46f

chore: update brisk-reconciler version in js.json

1db0231

fix: bug in Container rendering-logic

4c68021

refactor: update interface files per review

9516740

chore: update lock-files

42c073b

Et7f3 reviewed Nov 3, 2019

View reviewed changes

lessp changed the title ~~chore: convert examples and some components~~ refactor!: update to latest brisk-reconciler Nov 3, 2019

Et7f3 and others added 5 commits November 3, 2019 20:52

open Revery_UI.{React.Hooks,Animated}, add buildDev. Part-revert 245f46f

b225f60

Fix doc comments.

4579355

Fix rei don't access internal module__.

5fcf658

chore: formatting

8e157f0

Remove buildDev because it break the CI.

8167d9c

bryphe mentioned this pull request Nov 8, 2019

[Experiment] brisk-reconciler component PPX #566

Closed

bryphe approved these changes Nov 8, 2019

View reviewed changes

lessp changed the base branch from component-ppx to master November 8, 2019 20:01

Et7f3 reviewed Nov 10, 2019

View reviewed changes

src/UI_Components/Positioned.re Outdated Show resolved Hide resolved

lessp added 4 commits November 10, 2019 09:43

chore: remove unecessary module assignment

a47c9dc

chore: remove module assignment

56e6c33

chore: merge master locally

3fae6fd

chore: integrate recent changes from master

cb0c1c5

bryphe merged commit 700ecc7 into revery-ui:master Nov 11, 2019

bryphe mentioned this pull request Nov 11, 2019

Dependency: Revery -> 0.29.0 revery-ui/revery-quick-start#49

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: update to latest brisk-reconciler #576

refactor!: update to latest brisk-reconciler #576

lessp commented Oct 1, 2019 •

edited

Loading

lessp commented Oct 3, 2019

glennsl Oct 28, 2019

lessp Oct 28, 2019 •

edited

Loading

lessp commented Oct 28, 2019 •

edited

Loading

Et7f3 commented Oct 29, 2019

lessp commented Oct 29, 2019 •

edited

Loading

Et7f3 commented Oct 29, 2019

Et7f3 Nov 3, 2019

Et7f3 Nov 3, 2019

lessp Nov 3, 2019

Et7f3 Nov 3, 2019

Et7f3 Nov 3, 2019

lessp Nov 3, 2019

lessp Nov 3, 2019

Et7f3 Nov 3, 2019

Et7f3 Nov 3, 2019

lessp commented Nov 3, 2019 •

edited

Loading

Et7f3 commented Nov 3, 2019 •

edited

Loading

bryphe commented Nov 8, 2019 •

edited

Loading

bryphe left a comment

wokalski commented Nov 8, 2019

lessp commented Nov 9, 2019

wokalski commented Nov 9, 2019

bryphe commented Nov 11, 2019

refactor!: update to latest brisk-reconciler #576

refactor!: update to latest brisk-reconciler #576

Conversation

lessp commented Oct 1, 2019 • edited Loading

lessp commented Oct 3, 2019

Choose a reason for hiding this comment

lessp Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

lessp commented Oct 28, 2019 • edited Loading

Ok, so with a few files commented out:

Et7f3 commented Oct 29, 2019

lessp commented Oct 29, 2019 • edited Loading

Et7f3 commented Oct 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lessp commented Nov 3, 2019 • edited Loading

Et7f3 commented Nov 3, 2019 • edited Loading

bryphe commented Nov 8, 2019 • edited Loading

bryphe left a comment

Choose a reason for hiding this comment

wokalski commented Nov 8, 2019

lessp commented Nov 9, 2019

wokalski commented Nov 9, 2019

bryphe commented Nov 11, 2019

lessp commented Oct 1, 2019 •

edited

Loading

lessp Oct 28, 2019 •

edited

Loading

lessp commented Oct 28, 2019 •

edited

Loading

lessp commented Oct 29, 2019 •

edited

Loading

lessp commented Nov 3, 2019 •

edited

Loading

Et7f3 commented Nov 3, 2019 •

edited

Loading

bryphe commented Nov 8, 2019 •

edited

Loading