Skip to content
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

Component docs update: an effective compromise re: state & syntactic variance #2294

Merged
merged 9 commits into from
Nov 15, 2018
Merged

Component docs update: an effective compromise re: state & syntactic variance #2294

merged 9 commits into from
Nov 15, 2018

Conversation

CreaturesInUnitards
Copy link
Contributor

@CreaturesInUnitards CreaturesInUnitards commented Nov 13, 2018

Description

Fixes #2293, for v2 scope. Mostly just re-arranged the document flow, and special-cased ES6 classes.

Motivation and Context

Clarity on this doc has been severely lacking. I think this is simpler to understand, without omitting anything.

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

Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor nits. I am skeptical of the language strongly discouraging POJO components, though, especially with the AFAIK unsupported objectivity it implies of the community at large.

docs/components.md Outdated Show resolved Hide resolved
docs/components.md Outdated Show resolved Hide resolved

#### POJO Component State

For POJO components the state of a component can be accessed three ways: as a blueprint at initialization, via `vnode.state` and via the `this` keyword in component methods.
It is generally accepted that a component with state that must be managed is best expressed as a closure. If, however, you have reason to manage state in a POJO, the state of a component can be accessed in three ways: as a blueprint at initialization, via `vnode.state` and via the `this` keyword in component methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like @pygy and maybe @StephanHoyer to chime in on this one. I'd prefer to not introduce this strong of an opinion against a particular supported component type when our opinion is already pretty clearly implied through the broad use of closure components instead. Also, [citation needed] for the "It is generally accepted that ..." part - I'm not aware of anyone who's done a survey to verify that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something shorter and less contentious:

Whereas state management in closure components is a simple matter of scoped reference, in the case of a component defined as an object,

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm great with others chiming in, but I strongly disagree that this is a "strong...opinion against" the use of component state in POJOs. This is a strong advocacy for using closures ("best expressed as..."), but there's nothing negative suggested to the contrary, nor are there any of the warning flags we've been joking about on gitter.

I believe this is the kind of confident opinion which our docs are sorely lacking, TBF. This is proper guidance, essentially in the same form which we all use ad nauseam on gitter when someone asks.

Scenario: A random person pops in and says, "I have a component which needs to handle some internal state. What's the best approach?"

My hypothesis: About ten people — @isiahmeadows and @barneycarroll included — will jump in and recommend a closure (within minutes, if not moments).

Of course there's been no survey. But the easily-verified evidence on our gitter channel, from those in the community who are most engaged with the framework, speaks volumes.

I think the docs should have the courage of our convictions, rather than strive to achieve some sort of consensus. It's a form of design/write/code by committee which is too broad and never ends well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better yet, you could recommend that POJO components should be used for stateless components. Something like this:

It's generally recommended that POJO components should be stateless. If, however, you need to access and/or modify a POJO component's state, it can be accessed in three ways: as a blueprint at initialization, via vnode.state, and via the this keyword in component methods.

This way, it's a little more positive and it doesn't discourage the common case of embedding read-only data within the component itself, but it more clearly recommends when it's most useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wouldn't mind classes being recommended for TS+JSX users (thanks to microsoft/TypeScript#21699 and microsoft/TypeScript#14789).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...as for TS + JSX, I'm not qualified to talk about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CreaturesInUnitards Maybe stateful relative to the DOM, but I'm talking stateful relative to the component itself. If your DOM integration only requires something like $(vnode.dom).plugin({...}) and you can just update it with, say, $(vnode.dom).plugin("action") (this is the case with Bootstrap, BTW), then you don't need to care about vnode.state.

Also, not all hooks have to be stateful:

  • onbeforeupdate serves the secondary purpose of allowing you to diff attributes and cancel updates.
  • onbeforeremove serves to let you do things like CSS transitions.
  • view is obvious.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with @CreaturesInUnitards proposal. As we now move away from POJO and class components (which I strongly support, although I currently only use POJOs for historic reasons), it might be ok to make POJO-Components less prominent.

As stated on Gitter, I'm against the notion of stateless components and the relation to POJO Components.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'm personally a little uncomfortable with the wording, but I'll go with it if everyone else is okay with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great, let's merge this.

docs/components.md Outdated Show resolved Hide resolved
dead-claudia and others added 3 commits November 13, 2018 15:41
Co-Authored-By: CreaturesInUnitards <[email protected]>
Co-Authored-By: CreaturesInUnitards <[email protected]>
Co-Authored-By: CreaturesInUnitards <[email protected]>
@@ -185,7 +185,7 @@ A big advantage of closure components is that we don't need to worry about bindi

#### POJO Component State

It is generally accepted that a component with state that must be managed is best expressed as a closure. If, however, you have reason to manage state in a POJO, the state of a component can be accessed in three ways: as a blueprint at initialization, via `vnode.state` and via the `this` keyword in component methods.
It is generally recommended that you use closures for managing component state. If, however, you have reason to manage state in a POJO, the state of a component can be accessed in three ways: as a blueprint at initialization, via `vnode.state` and via the `this` keyword in component methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this clarification now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barneycarroll @StephanHoyer Are you okay with this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, brilliant 👍. Thanks @CreaturesInUnitards — this may well be the single best thing to happen to Mithril since @pygy fixed the dynamic list algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

Aww shucks 😊

@dead-claudia
Copy link
Member

dead-claudia commented Nov 15, 2018

I'm pretty sure this has been sufficiently reviewed now. 😄

@dead-claudia dead-claudia merged commit 52ccea2 into MithrilJS:next Nov 15, 2018
@CreaturesInUnitards CreaturesInUnitards deleted the component-state-docs branch November 15, 2018 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed/Declined
Development

Successfully merging this pull request may close these issues.

Docs re: component state are both polymorphic and variadic, and that's not good
4 participants