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

Update value tracking on cousin radios #11028

Merged
merged 3 commits into from Nov 16, 2017

Conversation

Projects
None yet
6 participants
@jquense
Collaborator

jquense commented Oct 2, 2017

fixes #10739

I’m not sure how this managed to not get fixed in the last PR. I updated the fixture to ensure that its fully testing whether the regression is fixed. Unfortunately
it means that 15.6.2 didn’t get this fix, do we call it at this point or what is the policy on releasing fixes on previous majors?

@reactjs-bot

This comment has been minimized.

Show comment
Hide comment
@reactjs-bot

reactjs-bot Oct 2, 2017

Collaborator

Deploy preview ready!

Built with commit 7230e62

https://deploy-preview-11028--reactjs.netlify.com

Collaborator

reactjs-bot commented Oct 2, 2017

Deploy preview ready!

Built with commit 7230e62

https://deploy-preview-11028--reactjs.netlify.com

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 2, 2017

Member

Flow fails:


$ node ./scripts/tasks/flow
Error: src/renderers/dom/shared/inputValueTracking.js:19
 19: type ElementWithValueTracker = HTMLInputElement & WrapperState;
                                                       ^^^^^^^^^^^^ property `_valueTracker`. Property not found in
333:       inputValueTracking.updateValueIfChanged(otherNode);
                                                   ^^^^^^^^^ HTMLInputElement. See: src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js:333

Unfortunately it means that 15.6.2 didn’t get this fix, do we call it at this point or what is the policy on releasing fixes on previous majors?

Was it always broken or did it regress in 15.6.0? If it fixes a regression I guess we'll have to cut 15.6.3. But we need to make sure the fix is solid.

Member

gaearon commented Oct 2, 2017

Flow fails:


$ node ./scripts/tasks/flow
Error: src/renderers/dom/shared/inputValueTracking.js:19
 19: type ElementWithValueTracker = HTMLInputElement & WrapperState;
                                                       ^^^^^^^^^^^^ property `_valueTracker`. Property not found in
333:       inputValueTracking.updateValueIfChanged(otherNode);
                                                   ^^^^^^^^^ HTMLInputElement. See: src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js:333

Unfortunately it means that 15.6.2 didn’t get this fix, do we call it at this point or what is the policy on releasing fixes on previous majors?

Was it always broken or did it regress in 15.6.0? If it fixes a regression I guess we'll have to cut 15.6.3. But we need to make sure the fix is solid.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Oct 2, 2017

Collaborator

This is was a regression in 15.6.0 yeah. grr flow, sorry about that, i have to much trust that my vscode will surface an issue :P

I've love to ensure that this solid, any other thoughts on acceptance criteria for that? We'll do the full fixture test run on it of course (not that that helped last time), more context here: #10186 (comment)

Collaborator

jquense commented Oct 2, 2017

This is was a regression in 15.6.0 yeah. grr flow, sorry about that, i have to much trust that my vscode will surface an issue :P

I've love to ensure that this solid, any other thoughts on acceptance criteria for that? We'll do the full fixture test run on it of course (not that that helped last time), more context here: #10186 (comment)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 2, 2017

Member

Do we have a post mortem on why fixtures didn't catch this?

Member

gaearon commented Oct 2, 2017

Do we have a post mortem on why fixtures didn't catch this?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Oct 2, 2017

Member

do we call it at this point or what is the policy on releasing fixes on previous majors

I think this would warrant another 15 patch release. IIRC we've done patch releases for a previous major before.

Member

aweary commented Oct 2, 2017

do we call it at this point or what is the policy on releasing fixes on previous majors

I think this would warrant another 15 patch release. IIRC we've done patch releases for a previous major before.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Oct 9, 2017

Collaborator

Do we have a post mortem on why fixtures didn't catch this?

Not entirely, I don't see much suggesting that something changed since the PR that broke it again, but idk. My hunch is that it's because the fixture was incomplete. It only asked to tests for 2 change events, the last fix essentially added one more change event where their wasn't one, but stopped there. so:

  • originally: you got one change event e.g initial selected A -> B worked, but not B -> A
  • first fix: A -> B -> A worked (2 events) but not A -> B -> A -> B
  • this one: all events fire
Collaborator

jquense commented Oct 9, 2017

Do we have a post mortem on why fixtures didn't catch this?

Not entirely, I don't see much suggesting that something changed since the PR that broke it again, but idk. My hunch is that it's because the fixture was incomplete. It only asked to tests for 2 change events, the last fix essentially added one more change event where their wasn't one, but stopped there. so:

  • originally: you got one change event e.g initial selected A -> B worked, but not B -> A
  • first fix: A -> B -> A worked (2 events) but not A -> B -> A -> B
  • this one: all events fire
@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Oct 27, 2017

Collaborator

gonna fix this up. anyone want to do a bit more testing in addition to what I've done or we feel good about this?

Collaborator

jquense commented Oct 27, 2017

gonna fix this up. anyone want to do a bit more testing in addition to what I've done or we feel good about this?

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Nov 3, 2017

Collaborator

ping before 16.1.0 happens

Collaborator

jquense commented Nov 3, 2017

ping before 16.1.0 happens

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Nov 3, 2017

Collaborator

(I still need to rebase sorry)

Collaborator

jquense commented Nov 3, 2017

(I still need to rebase sorry)

jquense added some commits Nov 6, 2017

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 14, 2017

Collaborator

@jquense I'll do some QA on this, but I can't get to it until later today.

Still I'd like to get this merged in soon.

Collaborator

nhunzaker commented Nov 14, 2017

@jquense I'll do some QA on this, but I can't get to it until later today.

Still I'd like to get this merged in soon.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 15, 2017

Collaborator

Works in:

Firefox 47, 57
Safari 7.1, 8, 9, 10.1, 11
Chrome 42, 43, 44, 62
IE 9, 10, 11
Edge 16
iOS 8.4, 10.2, 11

On an unrelated note, the click event test fixtures fails in:

Safari 7.1, 8, 9
iOS Safari 8.4
Chrome 42

I'll file a separate issue for that, even if it's to conclude that we don't care (#11560)

Collaborator

nhunzaker commented Nov 15, 2017

Works in:

Firefox 47, 57
Safari 7.1, 8, 9, 10.1, 11
Chrome 42, 43, 44, 62
IE 9, 10, 11
Edge 16
iOS 8.4, 10.2, 11

On an unrelated note, the click event test fixtures fails in:

Safari 7.1, 8, 9
iOS Safari 8.4
Chrome 42

I'll file a separate issue for that, even if it's to conclude that we don't care (#11560)

@nhunzaker

@jquense This works well for me. Please merge at your leisure.

I would lobby heavily for this to hit 15.x. This effects multiple of the apps that I maintain, and the only option in the short term is to down-grade to 15.4.0.

I'll even nominate myself to take on the work of cutting that release if it means anything.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 15, 2017

Collaborator

Oh, and I ran prettier for you, so this should pass CI soon :)

Collaborator

nhunzaker commented Nov 15, 2017

Oh, and I ran prettier for you, so this should pass CI soon :)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 15, 2017

Member

I think it is extremely unlikely we'll do another 15.x at this point.

Member

gaearon commented Nov 15, 2017

I think it is extremely unlikely we'll do another 15.x at this point.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 15, 2017

Member

Looking at the fix though it does seem pretty small. If we broke it and you're willing to cut 15.6.4 and more in case it adds further breakage then maybe. :-)

Member

gaearon commented Nov 15, 2017

Looking at the fix though it does seem pretty small. If we broke it and you're willing to cut 15.6.4 and more in case it adds further breakage then maybe. :-)

@jquense jquense merged commit e0e9131 into master Nov 16, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Nov 16, 2017

Collaborator

thanks team! A 15.6.4 would benefit me as well if we can swing it, @nhunzaker let me know if I can help with that

Collaborator

jquense commented Nov 16, 2017

thanks team! A 15.6.4 would benefit me as well if we can swing it, @nhunzaker let me know if I can help with that

@jquense jquense deleted the really-fix-radios branch Nov 16, 2017

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 16, 2017

Collaborator

Awesome. I'll write up a check list. I'd also like to run it locally on a few of our apps at work and verify everything is 👌 in real apps, then circle back to the test fixtures with any mishaps.

Collaborator

nhunzaker commented Nov 16, 2017

Awesome. I'll write up a check list. I'd also like to run it locally on a few of our apps at work and verify everything is 👌 in real apps, then circle back to the test fixtures with any mishaps.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Nov 16, 2017

Collaborator

The first thing to do is send this out to 15-dev. @jquense is that something you are comfortable taking on?

Collaborator

nhunzaker commented Nov 16, 2017

The first thing to do is send this out to 15-dev. @jquense is that something you are comfortable taking on?

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Update value tracking on cousin radios (facebook#11028)
* fix radio updates

* Format fixtures and ReactDOMFiberInput
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment