adamhaile / surplus

High performance JSX web views for S.js applications
638 stars 26 forks source link

Unidirectional dataflow doesn't work well #19

Closed Pyrolistical closed 7 years ago

Pyrolistical commented 7 years ago

I see surplus-fn-data does 2 way bindings, but I don't think that way. I tried to do unidirectional dataflow with surplus.

const Name = (props) => {
  const {name, onchange} = props
  return <input type="text" value={name} onKeyUp={onchange} />
}

const name = S.data('');
function updateName({ target: { value } }) {
  name(value);
}
const view = S.root(() => (
  <div>
    name:{name()}
    <br />
    <Name name={name()} onchange={updateName} />
  </div>
));
document.body.appendChild(view);

loss-focus

Problem with this code is after I type a character into the input, it loses focus. I think what is happening is the view is being regenerated and a new input field is replacing the old one. vdom solves this by patching....

Is this just how surplus works, or can I do unidirectional dataflow?

griebd commented 7 years ago

Form my understanding of S and Surplus I thought that in your example there would be NO node regeneration at all, if there is my understanding needs to be revisited!!! :wink:

So, @adamhaile, my question is: Am I wrong and @Pyrolistical possible cause for his problem is right? Or is my understand of S and Surplus correct?

In any case, I don't see why his code shouldn't work correctly, maybe surplu-mixin-data has code to keep the focus?

adamhaile commented 7 years ago

Yeah, there shouldn't be any node regeneration happening. @Pyrolistical, is this the exact code you're running? I ask because it looks like what you posted is at least missing a signal dereference, in that value={name} should be value={name()}. Also, are you on the release version, the @beta branch, or git master? It's possible I broke something :).

mixin-fn-data does two-way binding, but S and Surplus are agnostic about that.

I'm away from a computer for the weekend, so may not be able to reply further until Monday.

griebd commented 7 years ago

ok, I was curious about that and went ahead and tested it in my environment. I'm using the beta version. The relevant code is:

const ttt = S.data("");
const tttUpdate = ({ "target": { value } }) => {
  console.log(value);
  ttt(value);
};

return <input className="form-input" type="text" value={ ttt() } onkeyup={ tttUpdate } />

And as a result everything is working as expected... focus is keep, logged and input values are correct... couldn't find a problem using the beta version!

ismail-codar commented 7 years ago

I have tested the following example it working. value={name} is incorrect it must be value={name()} and onKeyUp ..

const name = S.data('');
function updateName({ target: { value } }) {
  name(value);
}
const view = S.root(() => (
  <div>
    name:{name()}
    <br />
    <input value={name()} onKeyUp={updateName} />
  </div>
));
document.body.appendChild(view);
Pyrolistical commented 7 years ago

@ismail-codar you are right that works, my bad for not including the full repo case.

here it is. I run into the issue when input is extracted into a stateless component

const Name = (props) => {
  const {name, onchange} = props
  return <input type="text" value={name} onKeyUp={onchange} />
}

const name = S.data('');
function updateName({ target: { value } }) {
  name(value);
}
const view = S.root(() => (
  <div>
    name:{name()}
    <br />
    <Name name={name()} onchange={updateName} />
  </div>
));
document.body.appendChild(view);

loss-focus

adamhaile commented 7 years ago

Ah, ok, I see what's going on. The way you've written it, you call <Name /> with the value of name(). So that call needs to be re-run when name() changes, and <Name /> therefore creates a new <input />. What you want to do is pass the name signal into <Name />, so that only the property set value={name()} gets re-run. See below. All I've done is move the deref of name() into the body of <Name /> rather than its call site.

const Name = (props) => {
  const {name, onchange} = props
  return <input type="text" value={name()} onKeyUp={onchange} />
}

const name = S.data('');
function updateName({ target: { value } }) {
  name(value);
}
const view = S.root(() => (
  <div>
    name:{name()}
    <br />
    <Name name={name} onchange={updateName} />
  </div>
));
document.body.appendChild(view);
Pyrolistical commented 7 years ago

interesting...this is very subtle, but thank you!

maybe documentation could be better to handle this gotcha

Pyrolistical commented 7 years ago

Added surplus microapp https://github.com/Pyrolistical/microapps

ismail-codar commented 7 years ago

Hi @Pyrolistical which application you are using to create this gif.

image

Pyrolistical commented 7 years ago

@ismail-codar https://www.cockos.com/licecap/