adamhaile / surplus

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

Dynamic attributes vs static attributes #23

Closed ismail-codar closed 6 years ago

ismail-codar commented 7 years ago

Hi @adamhaile only small problem in beta3 npm install && npm run build & npm run test cannot be work from fresh copy. PR is not required please add "s-js": "^0.4.5" and as dependencies and "jasmine-core": "^2.6.0" to devDependencies in package.json.

Besides about this issue here is an example:

<input
        className={style({ border: 'none' })} //it uses https://typestyle.github.io/#/core/-style-
        type="text"
        onInput={updateInputData}
        value={props.value()}
      />

Current output:

    __input1 = Surplus.createElement('input', null, __);
   Surplus.S(function () {
        __input1.className = lib_1.style({ border: 'none' });
        __input1.type = "text";
        __input1.oninput = updateInputData;
        __input1.value =props.value();
    }

className and onInput is looks dynamic if we look at it by JSX but it is static if we look at by surplus. Only value={props.value()} is dynamic. type="text" is hardly static.

Correct output may be:

    __input1 = Surplus.createElement('input', null, __);
   __input1.className = lib_1.style({ border: 'none' });
   __input1.type = "text";
   __input1.oninput = updateInputData;
   Surplus.S(function () {
        __input1.value =props.value();
    }
griebd commented 7 years ago

Hi there, yes, I confirm that! If there are any dynamic data some/all of the static data gets include on the dynamic update path...

adamhaile commented 7 years ago

So this behavior is by design, which isn't to say it's necessarily right 😄 but there is a rationale.

All properties, static dynamic and spread, are put into a single computation so as to respect precedence. Take a JSX expression like this:

<div style={{ width: width() }} style-width="50%"></div>

Because the static style-width declaration comes after the dynamic style one, JSX semantics say it should always override the earlier definition, and the width should always be 50%. However, if we put the dynamic part into its own computation, it could run again (when width() changes) and override the static style-width declaration, violating precedence.

The cost of this strategy is that we may re-set some static properties. I ran some benchmarks to see if this cost was measurable, and I couldn't distinguish any. The browsers are pretty fast at figuring out that certain sets are no-ops.

This is mostly only a concern when we have spreads involved, because surplus removes redundant and overridden named properties at compile time (i.e., if you enter <input type="checkbox" type="text" />, surplus knows the first declaration is overridden and removes it).

Even with named properties, however, precedence can come into play:

<span innerText={foo()} textContent="bar"></span>

This should always be <span>bar</span>, even if foo() changes and we re-run the innerText assignment.

Anyway, that's the thinking here. If I'm wrong and there really is a perf cost to the redundant static sets, then I might try for something more complicated.

fn each get their own computation, by the way. This is because they may have their own state, which they don't expect to be blown away when an unrelated dynamic property re-runs.

adamhaile commented 7 years ago

Whoops, forgot to reply about the s-js dependency.

I moved s-js to a peerDependency. I should update the homepage to say you need to install it too. The reason for the shift was that I was seeing a pattern where if you installed surplus first, then s-js, you actually ended up with two installations of s-js, one under surplus/node_modules the other under node_modules/. This is a bad thing, because if you have two versions of s-js running in your app, they don't talk to each other. So views bound with one version didn't respect changes to data signals made from the other version.

ismail-codar commented 7 years ago

Dublicate module problem is the real. Following webpack configuration fixes this. Source: https://github.com/webpack/webpack/issues/2134#issuecomment-192579511

resolve: {
      alias: {
        's-js': path.resolve(path.join(__dirname, '..', 'node_modules', 's-js')) 
      }
    }

Another way users can be use S function from surplus with import { S } from 'surplus'. Not adding s-js to their own package.json and not using import S from "s-js" style. Of course if the s-js dependency of surplus.

griebd commented 7 years ago

OK, we are talking about two different things here... :wink: I'm addressing the static vs dynamic properties...

I see why you made it that way BUT is there a case where it makes sense or it's useful? In your examples I would say the developer, user of surplus, should correct his code, it will be misleading for any future developer who read it... (side note: I'm getting used to JSX now, until I started with surplus I always used some hyperhtml variant like m in mithril)

If there is no use case then I believe you should simple remove the static data from computations. Without any "try for something more complicated". Just let it clear in the docs that this kind of "wrong" code will result in unexpected behavior.

If there is a use case, then let things as they are!

ismail-codar commented 7 years ago

@griebd Yes it was a bit strange two different things. The main subject here dynamic vs static attributes. I did not open a new topic for other thing because it was a very small.

@adamhaile I do think your explain. I think you're right. Surplus is already fast. If the this feature is implemented can much more speed or not much. Is it worth it. Maybe we can try it with a benchmark in the future. It is not necessary at the moment. But this issue can be open for terms of awareness.

Also separation to dynamic vs static attributes can be hard in compile time. For example <span innerText={foo()} textContent="bar"></span> Main problem may be: what is foo! is it declared var foo = S.data(...) or var foo = ()=>{.........} Is a reactive variable or only method call.

adamhaile commented 7 years ago

@griebd Surplus tries hard to be declarative. "Declarativeness" is a really important quality in general for reactive programs, because order-of-execution is obscured by the runtime.

One way to define declarativeness is that derived values are dependent only upon current state, not on the history of how we got here.

So say we only put dynamic properties into a computation, not static, and we had the following bit of code:

const HalfWidth = (props, children) =>
    <div {...props()} style-width="50%">
        {children()}
    </div>

If we asked "what's the current value of style.width?" we wouldn't be able to answer it without knowing whether props() changed since the time the div was created. If it hasn't, then the style-width="50%" attribute will have set style.width to 50%. But if it has, and if when it did props() held something like { style: { width: "40%" } }, then style.width would now be 40%. So we can't answer the question without knowing the history of changes: has {...props()} re-run?

By putting all properties into the same computation, we solve the problem: style.width will always be 50%, no matter what changed or in what order. So the history of changes no longer matters.

This is a small example and, you're right, not necessarily an example of the best coding practices :). But in general, not having to worry about history is a huge deal, arguably the whole point of reactive programming. So surplus only deviates from declarativeness if there's a very good reason.

griebd commented 7 years ago

I think I see your point, maybe you though it obvious, but it wasn't for me until I spent some time on it... :stuck_out_tongue_winking_eye:

if props include a style.width at a later time and the static attribution is outside the computation it will be overridden! so that is why it needs to be keep there, right? so it gets overridden again with the static value...

or am I still missing something?

adamhaile commented 7 years ago

Yep, exactly 👍