Crank: Removing a `type` prop from an `<input>` doesn't remove the type from the DOM node

[GormanFletcher]

Reproduction

If I have an input with type="email" and I replace it with an untyped input, the input in the DOM retains the type attribute.

[brainkim]

This is working as designed. Crank doesn’t “remember” previous props so they can be removed if they’re missing from the next. The reasoning behind this is I wanted less “hidden virtual DOM state.” As an example of a benefit, the API decision makes controlled/uncontrolled props from React unnecessary. I can write more about this later, but the “workaround” is to just explicitly pass type={null} to the input element.

{toggle ? (
  <input type="email" placeholder="Email" />
) : (
  <input type={null} placeholder="Name" />
)}

If you have thoughts on this, let me know!

[GormanFletcher]

Ah! I forgot that from the docs.

For implementation, would it make sense to diff the attrs on the reused DOM node against the new render's props, from the DOM instead of tracked state?

I don't understand if that would prompt API changes - if that puts us all the way in controlled/uncontrolled territory. The current behavior is a tricky caveat to "declarative", coming from other frontend frameworks, and its effects can be subtle to detect, since carryover attrs won't always produce easily visible changes (like an input that looks correct but fails to validate).

An explicitly null prop works in small examples like your comment or the docs, where logically we're rendering different versions of the same semantic element. I stumbled on this with a more complex example: a login/signup component that shows/hides the extra signup fields, which happen to be structurally similar enough to cause DOM node reuse. The signup form prompts name, then email, while the login form prompts just for email. In my mind, I'm declaring that the name input "goes away" and the email field that's identical in both code paths stays as-is. I'm not mentally modeling it as a mutation of one single element.

Maybe the answer is to explicitly tell Crank about that mental model? Liberal application of crank-key? That fixes the behavior in my codesandbox, and means I don't have to think about how code paths interact under the hood. But choosing that solution, I'd wind up peppering it all over the place defensively without knowing if I really need it everywhere, which doesn't feel great. So I don't know.

Thoughts?

[GormanFletcher]

I overlooked a complicating factor: because my <input> is uncontrolled, when my component toggles and Crank mutates the <input> in-place, the input will retain anything the user has typed, even if that value is inappropriate for the new meaning of the <input>. Deleting attrs won't help that - it sounds like I definitely need to use crank-key in that case.

Needing to watch out for attr preservation still strikes me as tricky, but now I don't have a concrete use case to point to for it, so take that as you will.

[brainkim]

Ah, this is really interesting. I will admit that I didn’t consider how having stateless, explicit props could have an effect on accidental element reuse. The status quo from React where props which are missing in successive renders are deleted, masks the problem of accidental element reuse somewhat. What I like about it is that it causes fewer visible bugs. On the other hand, as you noticed, accidental element reuse still suffers from stale state bugs as well, so perhaps we’re just masking a problem which should be addressed at some point. I see good arguments for both sides right now.

I think there is a tension between JSX and JavaScript statements, insofar as the declarative nature of JSX works best with components which have a single yield/return position, so that the declarative bits are obvious to the developer. When you start yielding/returning in multiple conditional branches, what came before is less obvious, so tricky issues of element reuse and state preservation come into play.

I stumbled on this with a more complex example: a login/signup component that shows/hides the extra signup fields, which happen to be structurally similar enough to cause DOM node reuse.

My specific advice here would be to use keys to force the virtual element algorithm to consider the roots of the branches as different. For instance:

// in a component
if (signup) {
  yield (
    <form crank-key="signup">{/* ... */}</form>
  );
} else {
  yield (
    <form crank-key="login">{/* ... */}</form>
  );
}

As an alternative, you could abstract the two different forms as two separate components, which would essentially have the same effect. You might want to have some advanced logic where the email is preserved between the two forms, but that can be done without reusing the actual DOM nodes.

For implementation, would it make sense to diff the attrs on the reused DOM node against the new render's props, from the DOM instead of tracked state?

We could try that, but I would worry about properties/attributes added via manual DOM mutations being erased by Crank rendering, and I’m not sure it will fix the problem.

The current behavior is a tricky caveat to "declarative", coming from other frontend frameworks, and its effects can be subtle to detect, since carryover attrs won't always produce easily visible changes (like an input that looks correct but fails to validate).

Insofar as React suffers from accidental state re-use, they also suffer from this problem, albeit to a less severe extent.

---

One thought is, the decision to only update props which are explicitly defined was one which I made in a slightly cavalier fashion, because it’s a low-commitment decision in terms of the larger Crank picture. For instance, you could actually write a Renderer subclass which does track rendered props and delete missing ones from successive renders. The Crank core is actually agnostic about this, as it’s all done at the renderer level.

However, the question I have is, were you happy with uncontrolled/controlled (defaultValue/value) distinctions? Crank represents an extraordinary opportunity to do the small, technically breaking quality-of-life improvements over React, insofar as compatibility is a non-goal (e.g. className ➡ class). So if you think the old solution to this problem was better, or if you have a better way to tackle the problem, let me know. Like I said, this is a low-commitment decision, and I’m happy to entertain wild ideas about how to solve controlled/uncontrolled props.

[GormanFletcher]

I think you're right: if the framework reuses DOM nodes, then it's the developer's responsibility to hint to the framework when a node is no longer the "same" one, either through component barriers or crank-key. Maybe that means the rule is to always provide the hint for stateful elements (inputs naturally, but also any element whose ref gets manipulated, or contenteditable, etc.). Supplying crank-key on the wrapping <form> instead of the individual inputs does keep the code impact minimal. Is there any performance penalty on overusing crank-key?

I would worry about properties/attributes added via manual DOM mutations being erased by Crank rendering, and I’m not sure it will fix the problem.

Yep - if stateful components need the hints anyway, that change wouldn't help here. Though it would help stateless components. But I think there's value in Crank offering a API contract of "you can freely manipulate elements without fighting Crank over it". Under that notion, the current behavior is the right approach unless/until Crank wants to track props history internally. (I think that would be a better user experience, but right now I don't feel strongly enough about the matter to ask Crank to take on the complexity burden.) This contract might be worth elaborating in the docs to help explain the tradeoff vs other frameworks.

When you start yielding/returning in multiple conditional branches, what came before is less obvious, so tricky issues of element reuse and state preservation come into play.

So far I have a hefty amount of conditional yields in my codebase. Some of that is loading states (conditional on a variable being undefined ) that I know I can simplify by taking better advantage of await inside AsyncGenerators. Something like:

yield <p>Loading...</p>;
await getSomeData();
this.refresh();
for await (const props of this) {
  ...
}

I just need to wrap my data subscriptions in Promises that resolve on the first value. I'm debating whether I like this better than the docs' loading indicator pattern. The advantage I see is that a component's loading-ness is self-contained; RandomDog's parent has to know that RandomDog needs a loading state.

were you happy with uncontrolled/controlled (defaultValue/value) distinctions?

In React land I never used defaultValue - I always used an onChange to write to a variable, sometimes with value={foo} if I had already-available data. (I think that means "controlled", but I can never keep straight which is which.) So what Crank is doing now works fine for me. It feels simpler to me to make the input's behavior explicit in my code; it shows what I tell it and its state lives with the rest of my state.

Crank represents an extraordinary opportunity to do the small, technically breaking quality-of-life improvements over React, insofar as compatibility is a non-goal

I am totally on board with that. I'll chew on this as I keep building my app and see if I can think of proposals that break out of the dichotomy of React's approach.

[brainkim]

Woke up with this thought in my head. What if we could reuse the Copy tag as an arbitrary prop value which indicates it should be uncontrolled? The problem with missing props being uncontrolled is that it’s difficult to omit props in JSX expressions themselves.

controlled ? <input type="text" name="name" value={value} />  : <input type="text" name="name" />

// vs

<input type="text" name="name" value={controlled ? value : Copy} />

I’m working with a lot of React code and its kinda incredible how much accidental element reuse there is in the wild.

[GormanFletcher]

Interesting thought! I like it directionally, but I'm concerned that overloading the Copy component to be a magic prop value could be confusing. Setting an initial value would also be a little bit tricky, as users would need to set a has-already-rendered flag in their component to switch between value and Copy.

So we're looking for a way for the user to signal to Crank that the value should only be read once, and left alone every render thereafter. Maybe we need a container for for the value that Crank could look for? If the value is a string, make it controlled, if it's a container, make it uncontrolled. Maybe the container is a new Crank export, or maybe using symbols could work? Symbols are nice for being built-in to the language, but seem semantically like a stretch. They do have the advantage of making it natural to express if the uncontrolled value is ever issued an updated value, but I don't know if that's a realistic scenario.

[brainkim]

@GormanFletcher

I'm concerned that overloading the Copy component to be a magic prop value could be confusing.

Fair point. Typechecking this would also be weird.

Setting an initial value would also be a little bit tricky

This is a problem I have with <Copy /> elements in general, which is that if there was nothing that was rendered before you get nothing, so you have to be careful about not using it in the initial render. Which is why I’m mildly excited about implementing something like a crank-skip prop which is just a boolean prop which allows you to skip rerenders, while initial renders would just work as expected and you wouldn’t need any code detecting the initial render. I’d love to hear your thoughts on that.

---

Some other options:

• Make undefined passed to props uncontrolled.
  👍 It can be put in JSX expressions.
  👎 Forces people to use null and I hate when people distinguish null and undefined
  👎 A breaking change

• Make crank-skip or some other option conditionally pass an array/iterable of prop names, which should be skipped
  👍 A non-breaking change
  👍 Avoids the initial render problem
  👎 Not immediately noticeable when reading elements
  👎 Might be performance intensive and the code might have to go in renderer subclasses.

  • defaultValue stuff.
    👍 React compatability
    👎 Isn’t really cranky insofar as we care about execution and rerenders.

Hmmmmmmmmmm.

[GormanFletcher]

I've gone around and around writing and deleting possible approaches, rereading comments ^^ / comments / docs, and here's where I've landed:

The difference between crank-skip and <Copy /> seems pretty subtle to me, and I wonder if it's something that can be folded into one feature to keep the mental load down.

I do like folding idempotency logic into a prop rather than a component, since it keeps the JSX cleaner. Using a simple boolean prop is alluring.

Should crank-skip be called crank-idempotent, since it _doesn't_ skip the first render?

I really like the idea of using a prop to tell Crank to leave some of a component's props alone. crank-idempotent-props={[...]}?

I agree that (1) isn't appealing using the subtle null/undefined distinction. Maybe it'd be okay with a sentinel value? value={crank.skip}? But that leaves the onus on the component to track first-render.

I'm not a fan of the defaultValue approach since it feels pretty special-cased to only support the value prop. I'm not putting any weight on React compatibility beyond basic JSX; Crank's differences are already stark enough to require checking a lot of React knowledge at the door, and in my opinion leaning into that is where Crank will find the opportunities to set itself apart.

Could you help me understand the crank-skip receive-props-but-not-execute behavior? What's distinct about it from a hypothetical version of Copy that took children, like this, which passes props to all of the children, but throws away the result instead of updating the DOM?

function* Copy2(_props: { children: crank.Children; update?: boolean }) {
  let displayedChildren!: crank.Children;
  for (let { children, update } of this) {
    if (!displayedChildren || update) {
      displayedChildren = children;
    }
    yield displayedChildren;
  }
}

function* App(this: crank.Context): Generator<crank.Child> {
  let counter = 0;
  setInterval(() => {
    counter += 1;
    this.refresh();
  }, 1000);
  for (let _props of this) {
    yield (
      <>
        <p>{new Date().toString()}</p>
        <Copy2 update={counter % 5 === 0}>
          <p>{console.log("a side effect") ?? "arbitrary text to render"}</p>
          <p>{new Date().toString()}</p>
        </Copy2>
      </>
    );
  }
}

[brainkim]

Should crank-skip be called crank-idempotent, since it doesn't skip the first render?

I get what you’re saying about the name “skip” not reflecting initial renders, but I’m not sold on “idempotent” either. It’s a handful of letters, an uncommon word, and math nerds will probably nit about their definition of “idempotent” being slightly different than the programmer/RESTful definition (something to do with monoids).

One alternative could be “static,” as in crank-static and crank-static-props, insofar as you’re saying the rendered elements will not change, though they may be initially rendered. This would also align with transpilation and hoisting of static elements. There might be precedent for this kind of terminology.

We could also make things really complicated and have something like crank-static/crank-static-props for idempotent behavior, crank-skip-props for props that should be skipped regardless of whether it was the initial render, and crank-include-props for props that should be included regardless of static-ity or skipping. Of course, this sort of API complexity is exactly what I’m trying to avoid in all of my work, just brainstorming.

Could you help me understand the crank-skip receive-props-but-not-execute behavior?

Even if you yield/return the same children in a component, like the Copy2 component does, the problem is that Crank will recursively walk through the children and “execute” them anyways. I think React has a thing where referentially equal elements are not re-rendered, but I didn’t really like that idea. What this exactly means is that component functions in the child element tree will be re-executed, and we update the DOM to match the virtual elements. Copying or skipping or static-ing prevents this process from happening for subtrees and can represent a huge savings in terms of performance.

[GormanFletcher]

crank-static sounds good to me.

I wonder if crank-static-props props could be avoided via props spreading:

let myProps = {value: 'initial value'}
for (let _props of this) {
  yield <input {...myProps} />
  {value, ...myProps} = myProps;  // set a prop once and leave it alone on subsequent renders
}

An always-skip prop sounds to me like omitting a prop from the JSX, rather than novel framework behavior.

I'm struggling to understand the behavior of an always-include prop as distinct from normal JSX that passes a prop all the time. Maybe that's something I need to see a component code sample for to get it?

[brainkim]

I wonder if crank-static-props props could be avoided via props spreading:

This works today, but relies on the missing = uncontrolled behavior.

I'm struggling to understand the behavior of an always-include prop as distinct from normal JSX

Yeah I’m just thinking out loud. Part of this falls into umbrella of performance optimizations where there is a virtually infinite amount of variation in terms of what developers could want. We might want an element to not update its props, but update its children, or update its props but not its children, or update everything except this one prop, or update nothing except this one prop. Do we accommodate all these situations? How would we do this in a way that makes sense for JSX syntax?

It’s a more difficult problem than controlled/uncontrolled, but the concerns overlap a little bit.