New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix docs bug, advise against reusing `vnode.attrs` itself #2250

Merged
merged 5 commits into from Nov 28, 2018

Conversation

6 participants
@isiahmeadows
Copy link
Collaborator

isiahmeadows commented Oct 14, 2018

Edit: update per current patch.

Description

Basically added a bunch of new docs to clarify 1. how to correctly avoid restrictive interfaces, and 2. why the obvious is also wrong. I added the known issues people had with lifecycle hooks and I touched a little on better component design.

Motivation and Context

Fixes #1775
Fixes #1986

How Has This Been Tested?

N/A - it's docs-only

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@isiahmeadows isiahmeadows added this to the 2.0.0 milestone Oct 14, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 14, 2018

@MithrilJS/collaborators This is option 3, instead of #2219 or #2246. Does this sound okay?

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Oct 15, 2018

Yeah, I think those clarifications in the documentation are a good addition. I'd offer more feedback on the writing (I think it could be edited down a bit) but work is so busy I'm not going to have much time to help out until next month.

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Oct 15, 2018

Thanks for plowing on with this Isiah. I've got a few issues with this:

  1. We should refer to the bad practice as attribute-forwarding. Having a short hook up describe the thing will make it easier for people to isolate and identify the problematic aspect.
  2. The advice for generic interfaces advocating forwarding is still there, I think we should get rid of that or at least rewrite it to make it clear it applies exclusively to children.
  3. The transition component example is highly involved — not only does this demand the reader get to grips with the mechanism of class-based transition, it doesn't explain what unexpected behaviour will occur or why. Can you elaborate on that? Maybe there's a simpler way to illustrate the problem.
@cavemansspa

This comment has been minimized.

Copy link
Contributor

cavemansspa commented Oct 15, 2018

here's an augmented version of the flems in #1986 that demonstrates the gotcha.

not sure if that's easier to grok as an example?

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Oct 15, 2018

The problem cases are mysterious, contrived, difficult to illustrate without the reader first swallowing a fairly involved proposition of intent.

But the scenario Isiah's used doesn't actually pose any problems. We're told unspecified unexpected behaviour will occur, but put into practice the sample code seems fine to me (ignoring for a second the growing list bug and the fact oncreate needs to specify a delay or DOM query before mutating the DOM to avoid its instructions happening in the same tick as initial render).

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Oct 15, 2018

@cavemansspa can you remember the real world difficulty that prompted the simplified flems you created?

@cavemansspa

This comment has been minimized.

Copy link
Contributor

cavemansspa commented Oct 15, 2018

@barneycarroll -- in practice, i tend to avoid passing attrs through like that, so have not really experienced it directly. i was following the issue and put that together so that i understood it better and where it was stemming from.

it's one of those cases of: "it's not really a bug, however, it's not the behavior you'd expect, but it makes sense once you understand what's occurring".

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:docs-update branch from aace9ef to 9dd5533 Oct 15, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 15, 2018

@barneycarroll @cavemansspa I killed the "Avoid restrictive interfaces", offering it as an implied option instead of an explicit suggestion. Here's how it currently reads, since the diff is fairly unrepresentative of the real semantic change here. (And of course, here's the section I ripped out.)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 15, 2018

Mithril shouldn't be that opinionated anyways, so I'm fine ripping it out.

@isiahmeadows isiahmeadows requested a review from MithrilJS/collaborators Oct 15, 2018

@cavemansspa

This comment has been minimized.

Copy link
Contributor

cavemansspa commented Oct 16, 2018

@isiahmeadows -- how about combining the code block and some inline comments to highlight the issue a little better?


This component implementation and use below demonstrates where you may unexpectedly get lifecycle methods invoked twice.

// AVOID
var FlexibleComponent = {
	view: function(vnode) {
		return m("button", vnode.attrs, [ // <= the lifecycle properties passed via attrs will be invoked a second time.
			"Click to ", vnode.children
		])
	}
}

m(FlexibleComponent, {
       // the lifecycle methods here will be invoked as expected.
	oncreate: function(vnode) {
		vnode.dom.ontransitionend = function() {
			vnode.dom.classList.remove("fade-in")
			vnode.dom.ontransitionend = null
		}
		vnode.dom.classList.add("fade-in")
	},
	onbeforeremove: function(vnode) {
		return new Promise(function(resolve, reject) {
			vnode.dom.classList.remove("fade-in")
			vnode.dom.classList.add("fade-out")
			vnode.dom.ontransitionend = function() {
				vnode.dom.classList.remove("fade-out")
				vnode.dom.ontransitionend = null
				resolve()
			}
		})
	}
})

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:docs-update branch from 9dd5533 to ac62bc3 Oct 17, 2018

vnode.dom.classList.add("fade-in")
},
onbeforeremove: function(vnode) {
// This gets called twice, both times awaited!

This comment has been minimized.

@StephanHoyer

StephanHoyer Oct 18, 2018

Member

maybe add

Once for the component itself, once for the button `FlexibleButton` which uses the same `attrs`.

This comment has been minimized.

@isiahmeadows

isiahmeadows Oct 18, 2018

Collaborator

Probably a good idea to add that "why". Will fix.

This comment has been minimized.

@barneycarroll

barneycarroll Oct 28, 2018

Member

You would still have to explain why this would be a problem, a question to which there is no answer.

]
```

Instead, what you should do in these cases is use a dedicated attribute like `attrs`. So the above two problematic examples should be fixed to something like these three:

This comment has been minimized.

@StephanHoyer

StephanHoyer Oct 18, 2018

Member

I only see one

This comment has been minimized.

@isiahmeadows

isiahmeadows Oct 18, 2018

Collaborator

That would be an artifact of a previous version. I'll fix it hopefully this weekend (if things go according to plan).

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Oct 18, 2018

made some comments

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Oct 18, 2018

Sorry, I think my message got lost in the noise. The example provided is bad because it doesn't actually produce the bug it claims to illustrate, while providing distracting examples of an involved transition mechanism which doesn't work (and would need to be more involved in order to work).

Here's a version with fixed transition logic (terminal / resting states, delay between render pass and oncreate DOM manipulation). As you can see, this doesn't produce the bug it's intended to - attribute forwarding is actually fine in this scenario.

@cavemansspa @spacejack I thought we had an open bug for the render -> oncreate delay expectations - did we decide against?

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Oct 18, 2018

I'm not sure what you mean by delay expectations - if you mean the "readyness" of the DOM to apply transitions I think the decision was against since userland solutions can be simpler than a general purpose one in the core. (Your flems url is broken BTW.)

@cavemansspa

This comment has been minimized.

Copy link
Contributor

cavemansspa commented Oct 18, 2018

@barneycarroll -- are your referring to #1779 discussion a while back for proposing vnode.ondomready?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 18, 2018

@barneycarroll I didn't test either example, so it's possible they're wrong. My goal is to produce in both cases a bug repro that's 1. reproducible, 2. easy to explain, and 3. relatable in the real world. I might just end up copying and reducing #1775's repro for the attrs issue, but I still need to test the key behavior to have a verified buggy example. (I might just recast that section to reuse the same component between both and rework the first to also provide buggy behavior for the second.)

I do plan on revisiting this over the weekend (once I have some more free time), so I'll try to fold some of these suggestions in then.

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Oct 28, 2018

OK, never mind the oncreate same-tick issue — that's an incidental problem of implementing transitions in the code @isiahmeadows happened to use. It's not really of consequence to the broader problem.

Let's put it another way: we're currently describing this as a footgun for what would appear as bugs, ie the library behaving in unexpected ways, but this is a category error: so far nobody has been able to come up with a tangible problem case. @isiahmeadows' example doesn't illustrate a problem: users could take that pattern and run with it and would never run into any problems; @cavemansspa's example does illustrate the potential for expectation shortfall but it's so abstract that it doesn't really prove anything — if we were to use our imaginations to flesh it out into something with discernible intent (while still illustrating some kind of problem), it would likely be a case of using lifecycle hooks for non-idempotent application logic — which would be a case of relying on Mithril redraw engine logic for side effects — a blatant anti-pattern. My point is that there's no way of presenting this as an implementation detail gotcha without producing a very elaborate problem case which would have to be a fundamentally bad idea of itself.

In fact, the real problem with attribute-forwarding is a more subtle one of extensible component interface design, when authors are tempted to get the best of both worlds from a single object: a custom interface that addresses certain context-specific concerns, and the underlying 'generic' interface. Initial convenience leads to intractable ambiguity down the line. This is a perennial problem in AOP interface design most redolent of the ambiguities of subclassing (Should I call super when I override this method? Should I call it before or after my subclassed behaviour?). In DOM component design the problem comes when you want to provide a mix of a custom component attribute interface while leaving the door open for generic DOM attribute customisation: the classic example is some kind of Input component which encapsulates your UI's convention for a DOM structure involving label, input, conditional tooltips & errors, structural elements, with a simplified interface for some application conventions (eg size: large | medium | small or priority: urgent | main | optional | normal standing in for involved internal styling concerns, an abstracted interface to manage model binding, etc) — but that also forwards those attributes which aren't part of the component-specific interface to the underlying input element as an escape hatch — ie secondary author can reuse the same component while specifying extra classes, styles, event handlers to handle special variations on a case-by-case basis. This is when all hell slowly but inexorably breaks loose because what started off as a well-intentioned simplification API gradually becomes exponentially more complicated than a non-abstracted or properly composed interface.

And the solution to that is simply never to forward the attributes object or the filtered remains thereof to a descendant node: where this is desirable the interface author should earmark specific custom attributes (in this case inputAttrs or labelAttrs or whatever) to indicate those; but even then — will the passed in hooks which clash with component-determined hooks (class, style, event handlers) take precedence? Be overwritten?

It's a code smell, not a technical gotcha footgun.

TL;DR: it's complex, subtle & involved, and no explanation is IMO better than a confusing gesture towards the problem domain.

In the absence of an explanation which can clearly illustrate tangible problems in less than 500 words + 100 LoC I vote we just nerf this section of the docs until we come up with something better.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 28, 2018

@barneycarroll I'd still rather find a case where it introduces bugs in user code, and the two linked bugs I'm resolving with this PR are my main driving reason for including this.

@cavemansspa's example does illustrate the potential for expectation shortfall but it's so abstract that it doesn't really prove anything — if we were to use our imaginations to flesh it out into something with discernible intent (while still illustrating some kind of problem), it would likely be a case of using lifecycle hooks for non-idempotent application logic — which would be a case of relying on Mithril redraw engine logic for side effects — a blatant anti-pattern.

oncreate and onremove shouldn't have to be idempotent - the docs say they are only run once per vnode. And most people, myself included, make the assumption that I don't have to worry in oncreate about the element already being initialized or in onremove about the element already being detached.

I'll try to find a better scenario that correctly runs into issues here once I find time.

In fact, the real problem with attribute-forwarding is a more subtle one of extensible component interface design, [...]

BTW, not everyone agrees with your assessment that attribute forwarding is itself inherently bad - @tivac and I are a "sometimes", and @lhorie, who wrote the section, was a "whenever possible". If we've got three different, common viewpoints on the issue and nobody agrees on any of it, I find it better to just rip out the controversial "advice" altogether, without replacement. It's also why I phrased it as an "if" when explaining attribute forwarding, with no implication on whether you should.

TL;DR: it's complex, subtle & involved, and no explanation is IMO better than a confusing gesture towards the problem domain.

This is part of my difficulty in coming up with something "that's 1. reproducible, 2. easy to explain, and 3. relatable in the real world".

In the absence of an explanation which can clearly illustrate tangible problems in less than 500 words + 100 LoC I vote we just nerf this section of the docs until we come up with something better.

I'm good with this as a first step, and then I'll rebase this PR. Edit: #2265

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 28, 2018

@isiahmeadows isiahmeadows referenced this pull request Oct 28, 2018

Merged

Remove a section that's 1. buggy and 2. controversial #2265

1 of 11 tasks complete

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 28, 2018

@isiahmeadows isiahmeadows changed the title Fix docs bug, advise against reusing `vnode.attrs` itself Fix docs bug, advise against reusing `vnode.attrs` itself (don't merge) Oct 28, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 28, 2018

I'm putting this on hold until #2262 gets resolved, since this doesn't really make nearly as much sense without that.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 13, 2018

@CreaturesInUnitards Does this intersect with #2294?

@CreaturesInUnitards

This comment has been minimized.

Copy link
Contributor

CreaturesInUnitards commented Nov 15, 2018

@isiahmeadows sort of — all the stuff about forwarding attrs and gakking up keys is gone, so in that sense, no :)

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:docs-update branch from aaf7d51 to 7111231 Nov 15, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 15, 2018

Okay, I recast the section entirely with a different example (in this case, a case of third-party integration gone wrong). I also removed the part about keys, and just strictly focused on lifecycle methods and touched a little on component design.

It's a little large, but that's because I went with a full Bootstrap modal component and dissected the two main problems within it.

@isiahmeadows isiahmeadows requested a review from MithrilJS/collaborators Nov 15, 2018

@CreaturesInUnitards

This comment has been minimized.

Copy link
Contributor

CreaturesInUnitards commented Nov 15, 2018

Just my 2 cents: one thing that jumps out to me in these code examples is the array notation for children. I think it adds visual noise and confusion. It wouldn't surprise me to find out that I'm in the minority, but FWIW if if were up to me we would only recommend and/or demonstrate square brackets as the root of a view.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 19, 2018

@CreaturesInUnitards I'm a creature of habit, but there's a few other reasons I did this:

  1. 99% of the rest of the docs use this for multi-line vnodes*, so I'd rather remain consistent.
  2. I like to keep a cleaner separation between attrs and children, and on multi-line vnodes, it's much easier to maintain this separation with an array literal. It's a little more verbose, but it's more readable to me personally.
  3. Prior to ES2017, you couldn't use trailing function parameter commas (which are useful to avoid diff noise), and old habits die hard. 🙂

We can change the docs to prefer variadic parameters, but I'd prefer to keep that discussion in its own issue.

* Except when the children is a single list.map(function (item) { return vnode }) - the few cases this is used, the result is just returned directly.

@isiahmeadows isiahmeadows changed the title Fix docs bug, advise against reusing `vnode.attrs` itself (don't merge) Fix docs bug, advise against reusing `vnode.attrs` itself Nov 22, 2018

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:docs-update branch from 6ffcb0f to e164c6e Nov 22, 2018

@StephanHoyer
Copy link
Member

StephanHoyer left a comment

The example you use is pretty complex. Couldn't we just do a simple example like this?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 27, 2018

@StephanHoyer The reason I redid it was because @barneycarroll wanted a concrete case of where it breaks things. Pretty much everyone who ran into bugs doing this had a layer of abstraction that broke elsewhere, so I had to create that layer of abstraction and indirection to show where it goes wrong. The example also touched on idiomatic API design. I'll kill the header/body/footer attrs customization, which should help simplify it some. Those aren't critical to explain this.

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:docs-update branch from e164c6e to 8b0406e Nov 27, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 27, 2018

@StephanHoyer I removed code for the unused parameters, if that helps.

@StephanHoyer
Copy link
Member

StephanHoyer left a comment

The example is still hard to follow for me. Maybe it's because I never used jquery or bootstrap to such an extend.

}
```

It's also less flexible. For example, you can't assign custom classes to your `.modal-header` element itself:

This comment has been minimized.

@StephanHoyer

StephanHoyer Nov 27, 2018

Member
Suggested change Beta
It's also less flexible. For example, you can't assign custom classes to your `.modal-header` element itself:
It's also less flexible. For example, you can't assign a custom id to your `.modal-header` element itself:
@StephanHoyer
Copy link
Member

StephanHoyer left a comment

The example is still hard to follow for me. Maybe it's because I never used jquery or bootstrap to such an extend.

@StephanHoyer
Copy link
Member

StephanHoyer left a comment

The example is still hard to follow for me. Maybe it's because I never used jquery or bootstrap to such an extend.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 27, 2018

@StephanHoyer That might be it. Do you have any other libraries that might do the trick? I'd be willing to replace this with a generic vanilla modal library (I don't know of any right off) that would do the same.

I'm just used to people being familiar with jQuery and at least some of its ecosystem because it powers like 90% of the web (including Wordpress, Drupal, and Rails for core client-side management) and it's about the only library beyond Bootstrap that designers are generally familiar with.

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Nov 27, 2018

I only use vanilla DOM with a drag-drop lib. Other than that, I always render stuff myself.

I also always never faced that issue, I was never tempted to forwar vnode.attrs. So I might be the wrong person to review this ;)

But feel free to merge as is.

@isiahmeadows isiahmeadows force-pushed the isiahmeadows:docs-update branch from b3fae4e to 870e55e Nov 27, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 27, 2018

@StephanHoyer Is this one easier to follow? It still uses jQuery and Bootstrap, but I kept it more focused on Mithril and I slimmed it down to about half the size.

@StephanHoyer
Copy link
Member

StephanHoyer left a comment

much better to follow! 👍

@StephanHoyer StephanHoyer merged commit 7d8a889 into MithrilJS:next Nov 28, 2018

Triage/bugs automation moved this from Low priority to Closed Nov 28, 2018

@isiahmeadows isiahmeadows deleted the isiahmeadows:docs-update branch Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment