-
-
Notifications
You must be signed in to change notification settings - Fork 924
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
dead-claudia
merged 9 commits into
MithrilJS:next
from
CreaturesInUnitards:component-state-docs
Nov 15, 2018
Merged
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0682167
Update components.md
CreaturesInUnitards 0505902
Update components.md
CreaturesInUnitards c8ead19
Update components.md
CreaturesInUnitards 2f03e4c
Update components.md
CreaturesInUnitards 07b6fd1
Update docs/components.md
dead-claudia adde1cd
Update docs/components.md
dead-claudia 9aa65b3
Update docs/components.md
dead-claudia a5bf6b1
teeny clarification
CreaturesInUnitards 811696c
reasonably compromised language, I think
CreaturesInUnitards File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 aboutvnode.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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.