reasonml-old / reason-react Goto Github PK

TODO: https://gist.github.com/ckknight/e4fb973201d82d4b3d049aa4de037c76

From @ckknight. We'll make this official

This affects to prop type, with type_, and possibly other labels, now going untranslated. Should at some point be changed to _type I think.

See:
http://bloomberg.github.io/bucklescript/Manual.html#_object_label_translation_convention

There is integration with type-script. Any plans for something similar for Reason/Bucklescript?

UPD: for ClojureScript. See also crra

Right now, it's not even considered valid syntax by the JSX ppx.

See:
https://github.com/facebook/react/blob/b59fb796ea2c05c8609858fc28b707d13245fd9c/docs/docs/reference-dom-elements.md#all-supported-html-attributes

Reason React currently defines style in ReactDOMRe. In bs-react-native we have also defined styles.

Now we have a problem if some component is cross-platform (e.g. react-navigation) and takes style as a prop (for which style would you write the bindings?).

I would propose to move the style type from ReactDOMRe to ReactRe. Then we could use the same style in both projects.

logs pasted below

git>git clone https://github.com/reasonml/rehydrate
Cloning into 'rehydrate'...
remote: Counting objects: 286, done.
remote: Compressing objects: 100% (78/78), done.
remote: Total 286 (delta 30), reused 0 (delta 0), pack-reused 205
Receiving objects: 100% (286/286), 51.10 KiB | 0 bytes/s, done.
Resolving deltas: 100% (128/128), done.
Checking connectivity... done.
git>cd rehydrate/
rehydrate>time npm install
npm ERR! git clone --template=/Users/hzhang295/.npm/_git-remotes/_templates --mirror git://github.com/npm-opam/easy-format.git /Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929: Cloning into bare repository '/Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929'...
npm ERR! git clone --template=/Users/hzhang295/.npm/_git-remotes/_templates --mirror git://github.com/npm-opam/easy-format.git /Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929: fatal: unable to connect to github.com:
npm ERR! git clone --template=/Users/hzhang295/.npm/_git-remotes/_templates --mirror git://github.com/npm-opam/easy-format.git /Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929: github.com[0: 192.30.253.113]: errno=Operation timed out
npm ERR! git clone --template=/Users/hzhang295/.npm/_git-remotes/_templates --mirror git://github.com/npm-opam/easy-format.git /Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929: github.com[1: 192.30.253.112]: errno=Operation timed out
npm ERR! Darwin 15.6.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "install"
npm ERR! node v7.2.0
npm ERR! npm  v3.10.9
npm ERR! code 128

npm ERR! Command failed: git clone --template=/Users/hzhang295/.npm/_git-remotes/_templates --mirror git://github.com/npm-opam/easy-format.git /Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929
npm ERR! Cloning into bare repository '/Users/hzhang295/.npm/_git-remotes/git-github-com-npm-opam-easy-format-git-1-2-0-bfe84929'...
npm ERR! fatal: unable to connect to github.com:
npm ERR! github.com[0: 192.30.253.113]: errno=Operation timed out
npm ERR! github.com[1: 192.30.253.112]: errno=Operation timed out
npm ERR! 
npm ERR! 
npm ERR! 
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/hzhang295/git/rehydrate/npm-debug.log

real	1m39.491s
user	0m8.724s
sys	0m2.725s

Before we reach the tweet about this-phase, we should provide a way to easily integrate this into existing projects. I'm now manually copying files, which does pretty much not scale at all.
The approach I'm going to follow now is dumping all output into one flat folder, which I'm going to alias through webpack for easy access.

Thoughts?

This repo is a good test case for bsb's merlin generation. We can call merlin and and pipe in some lines and assert on the output, to make sure the .merlin is configured correctly.

I believe we need (at least the following)

• Better package.json with env commands to be able to launch editor with perfectly constructed path.

• ninja as an npm dependency.

At the moment, ReactRe.Component exposes the updater wrapper, to give an up-to-date component bag to the callback (and memoize the callback) and to appropriately handle state changes from callbacks.

There is a wish for a simplified version of this wrapper for in stateless components, that only provides the props and expects an unit return value (and as such, will not do any state updates). Taking the current rehydrate docs for updater as a sample (and a temporary name of handler), we could use it like this:

/* `props` is up-to-date here, even though onClick is asynchronously triggered */
let handleClick props event => {Js.log "clicked!"};
let render {props, updater} => <div onClick=(handler handleClick) />

Not sure what the root cause is, but noticed that
<span>(ReactRe.stringToElement "한국어")</span>
Is rendered as
<span>í��êµ&shy;ì�´</span>

I tried creating a new component like this, with a single labelled curried argument (similar to what's in the documentation, but with only one argument):

/* test.re */
module Test = {
  include ReactRe.Component;

  let name = "Test";
  type props = {
    title: string
  };

  let render {props} => {
    <div>
      <h1>(ReactRe.stringToElement props.title)</h1>
    </div>
  };
};

include ReactRe.CreateComponent Test;

let createElement ::title => wrapProps {title};

/* app.re */

/* ... */
  <Test
    title="a title"
  />;
/* ... */

This fails with the following error:

Event change
>>>> Start compiling
Rebuilding since [ 'change : test.re' ]
BSB check build spec : OK 
ninja.exe -C lib/bs 
ninja: Entering directory `lib/bs'
[2/2] Building src/todomvc/test.mlast.d
[2/2] Building src/todomvc/app.cmj /Users/karlhorky/projects/reason-react-experiments/lib/js/src/todomvc/app.js src/todomvc/app.cmi
FAILED: src/todomvc/app.cmj /Users/karlhorky/projects/reason-react-experiments/lib/js/src/todomvc/app.js src/todomvc/app.cmi 
/Users/karlhorky/projects/reason-react-experiments/node_modules/bs-platform/bin/bsc.exe -bs-package-name reason-react-example  -bs-package-output commonjs:lib/js/src/todomvc -bs-assume-no-mli -bs-no-builtin-ppx-ml -bs-no-implicit-include -I /Users/karlhorky/projects/reason-react-experiments/node_modules/reason-react/lib/ocaml -I /Users/karlhorky/projects/reason-react-experiments/node_modules/reason-js/lib/ocaml -I src/interop -I src/logo -I src/simple -I src/todomvc -I src  -nostdlib -I /Users/karlhorky/projects/reason-react-experiments/node_modules/bs-platform/lib/ocaml -no-alias-deps -color always -w -40+6+7+27+32..39+44+45 -o src/todomvc/app.mlast -c  src/todomvc/app.mlast 
File "/Users/karlhorky/projects/reason-react-experiments/src/todomvc/app.re", line 144, characters 19-31:
Error: This expression has type string but an expression was expected of type
         Test.Test.props
ninja: build stopped: subcommand failed.
>>>> Finish compiling

It does, however, work with two labelled curried arguments:

/* test.re */
module Test = {
  include ReactRe.Component;

  let name = "Test";
  type props = {
    title: string,
    description: string
  };

  let render {props} => {
    <div>
      <h1>(ReactRe.stringToElement props.title)</h1>
      (ReactRe.stringToElement props.description)
    </div>
  };
};

include ReactRe.CreateComponent Test;

let createElement ::title ::description => wrapProps {title, description};

/* app.re */

/* ... */
  <Test
    title="a title"
    description="a description"
  />;
/* ... */

https://facebook.github.io/react/docs/uncontrolled-components.html says that e.g. textarea supports defaultValue, but it's not in the list of allowed props.

For now the problem is that callback receives component instance state
and not the state provided by setState((prevState, props) => ...) callback arguments.

This can cause situations when handler can get outdated version of state.
The simples example to reproduce just call event handler twice inside setTimeout

// js
class Tmp extends Component {
  componentDidMount() {
      setTimeout(() => {
         this.props.onIncreaseCounter();
         this.props.onIncreaseCounter();
      }, 100);
  }
  ...
}

So using such component in reason using updater will update counter just once

let increaseCounter {state} event => Some {...state, counter: state.counter + 1 }
let render {updater} => {
   <Tmp onIncreaseCounter=(updater increaseCounter) />
}

For now seems like this problem is unsolvable.

as jordwalke said:

there's a problem in react right now that prevents us from actually using it.
Because if you use the callback version of setState, (in React) there's no opportunity for you to say "nevermind don't update state" after you examine the freshest state
I reported the issue to team mates

So until we get additional setState version in React, all will be as is.

In our current bindings, aria-foo and the likes aren't valid syntax for function labels, so we can't pass aria-* and data-* to a component.

Of course, you can always write a dedicated aria component! We should either design this away, or provide some magic.

It needs to accept either Node, or (DocumentFragment | Element), depending on what it actually needs (more likely the latter).

Discovered when trying to fit ShadowRoot into render, which isn't an Element but works perfectly fine with render if it pretends to be.

Also checked TypeScript's bindings, and they are equally wrong on this point. They do accept null though...

Isn't it always a DOM element anyway?

Something like "do you even functions" but a bit more polite

If the user likes to use the fancy build output option, then this should be the unifying UI for native and js.

We're currently doing string matches on bsb -w's output, which is hacky. Maybe we should just use this as a new front-end for bsb watcher, similar to bsb_watcher.js. Though bsb -w -make-world goes through a different path.

@bobzhang

This can be done in the component base somehow.

reasonml/reason-react#45 (comment)

See https://facebook.github.io/react/docs/events.html#reference

Both of these will compile (I hope, they've been modified a bit for clarity), but only the first will actually execute updater. The second example will simply leave it partially applied. No warning is given by the compiler.

let f : string => unit = fun value =>
  updater (fun { state } () => Some { ...state, value }) ();

let f : string => unit = fun value =>
  updater (fun { state } () => Some { ...state, value });

Reference of initial discussion: https://twitter.com/_jayphelps/status/842557134994788352

Lifecycles functions currently return option of state, so Some or None, which means they are expected to synchronously return state. In practice, a large number of components will want to make some form of asynchronous side effect (e.g. http request) and then update the state later.

One could use setState for this, however the documentation currently describes setState almost as if it were an anti-pattern, which presents confusion since async state is not rare, it's essential.

@chenglou suggested reason-react could adjust the return type to allow return of a Promise No | Sync 'state | Async (Promise 'state) however I feel this presents other problems like how does someone cancel pending async work when the component unmounts? One could also make an argument for Observable instead of Promise. There's even completely different paradigms for managing side effects reason-react could consider like how redux middleware works (redux-saga, redux-observable, etc) or things like cycle.js side effect drivers. There's a lot of potential options.

Mostly recording here so the Twitter convo isn't lost and people can start a discussion.

Here's a pretty standard JS-version of imperative async side effect + cancellation for reference:

class User extends Component {
  state = {
    user: null
  };

  componentDidMount() {
    const { id } = this.props;

    this.subscription = Observable.ajax.getJSON(`/users/${id}`)
      .subscribe(user => {
        this.setState({ user });
      });
  }

  componentWillUnmount() {
    // cancellation (or noop if already done)
    this.subscription.unsubscribe();
  }

  render() {
    const { user } = this.state;

    if (!user) {
      return <div>Loading...</div>;
    }
  
    return (
      <div>
        <h1>Name: {user.name}</h1>
        <p>Age: {user.age}</p>
      </div>
    );
  }
}

fresh git clone, running yarn gives this

➜  rehydrate git:(master) yarn
yarn install v0.15.1
info No lockfile found.
[1/4] 🔍  Resolving packages...
error Command failed.
Exit code: 128
Command: git
Arguments: pull
Directory: /Users/threepointone/.yarn-cache/.tmp/baf9413ad449f940390553af573f88c0
Output:
warning: no common commits
From git://github.com/npm-opam/easy-format
 + 1b3ab29...8bce8fb master     -> origin/master  (forced update)
 + 6d7cda1...8bce8fb latest     -> origin/latest  (forced update)
fatal: refusing to merge unrelated histories
    at SpawnError (/usr/local/Cellar/yarn/0.15.1/libexec/lib/node_modules/yarn/lib/errors.js:18:1)
    at ChildProcess.proc.on.code (/usr/local/Cellar/yarn/0.15.1/libexec/lib/node_modules/yarn/lib/util/child.js:107:15)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at maybeClose (internal/child_process.js:877:16)
    at Socket.<anonymous> (internal/child_process.js:334:11)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at Pipe._handle.close [as _onclose] (net.js:498:12)

worked fine with npm install

I haven't gone through the list but noticed that the global attribute "title" is missing as a prop on createElement. Is there a reason (hah!) for this, or can I just add them?

When trying to set the attribute in the following example, it throws a type error:

<button dangerouslySetInnerHTML={ "__html": "&times;" } />

Error:

Error: This expression has type < __html : string > Js.t
       but an expression was expected of type string

If this is a bug and should work, I would love to do the PR for it if I could get pointed in the right direction.

We can use the [@bs.obj] external.

We can loosen the requirement that "ReactJS children must be typed as Js.null_undefined ReactRe.reactJsChildren"

I think we use List.find to memoize - this throws exceptions in JS (to preserve implementation semantics) and this is not as fast when in the JS VM.

This is deprecated and we won't support it. But right now it fails silently.

We use currying for ref and key already. There might be some phantom types/GADT things we can use here to statically type the fact that children key exists. More info: https://github.com/reasonml/rehydrate/blob/6808bf1a19e701214e4456967b4aab155d79802c/src/reactRe.re#L60

Alternatively, @jordwalke can take the occasion to design this away without needing fancy types, with his OrderedMap idea (jordan can talk more about this).

The benefit is that this is the last feature we need in order to completely bypass createElement (which sets up the key warning, that we currently piggyback on), and make every JSX render twice faster.

No more setState-after-free errors!

createClass will be moved out of React core. We don't currently compile to es6 classes (the spec isn't even finalized), so we can take this occasion to also drop our internal usage of createClass, and wire things up into React ourselves. Page initialization cost should go down a bit.

Setting the step property of an input field to a float will currently give a compile-time error, not making it possible to step float fields in a more fine-grained fashion than integer values.