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

Editable.changeFormats() should update setState #1124

Closed
BE-Webdesign opened this issue Jun 11, 2017 · 6 comments
Closed

Editable.changeFormats() should update setState #1124

BE-Webdesign opened this issue Jun 11, 2017 · 6 comments
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Enhancement A suggestion for improvement.

Comments

@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented Jun 11, 2017

Currently:
Around line 368 inside changeFormats()

this.setState( {
	formats: merge( {}, this.state.formats, formats ),
} );

Should be:

this.setState( ( previousState ) => ( {
	formats: merge( {}, previousState.formats, formats ),
} ) );
@BE-Webdesign BE-Webdesign added Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Enhancement A suggestion for improvement. labels Jun 11, 2017
@aduth
Copy link
Member

aduth commented Jun 12, 2017

Can you explain the rationale on this one?

Or is this because Facebook has more recently advocated the function form as being more aligned with Fiber goals? Saddens me that it's a bit more verbose 😞

@aduth
Copy link
Member

aduth commented Jun 12, 2017

Noticed there's a handful more of these issues. Is there a parent issue? Should we create one?

@youknowriad
Copy link
Contributor

The logic I think is because setState is not guaranteed to be synchronous, so we're not sure to have the state in the proper shape if we call two synchronous setState calls that rely on the previous state.

@aduth
Copy link
Member

aduth commented Jun 12, 2017

Hmm, just doesn't seem like a gotcha that's commonly encountered. At least it's not relevant in this case.

I'm not particularly opposed to it, especially now that it's become more advocated as the preferred pattern. Some questions like: Is previousState the most accurate naming, since it's technically the "current" state, isn't it? Or maybe even state, assuming it's not already been assigned in the containing scope.

Was there some previous discussion of this?

@youknowriad
Copy link
Contributor

Was there some previous discussion of this?

I don't think so.

Wording

Maybe state is a good name to match Redux's reducers

@BE-Webdesign
Copy link
Contributor Author

Changing this actually fixes up some weird glitches I have around formatting. I would encounter things where the format was not applying correctly and after I switched the code over it worked perfectly.

Noticed there's a handful more of these issues. Is there a parent issue? Should we create one?

Yeah I just opened them to chronicle what I saw in a quick glance. Most of them probably will never cause an error, this one has for me. Whether we should create a parent issue is up to you. The benefit is minimal probably, but this is a best practice and will work better with the new way fiber is going to reconcile and update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

3 participants