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

CP-1743 Read remaining data from request buffer on xhr close #10

Merged
merged 11 commits into from
May 12, 2016

Conversation

davidmarne-wf
Copy link

@davidmarne-wf davidmarne-wf commented May 9, 2016

PROBLEM

On chrome when the HttpRequest's state is changed to DONE the onReadyStateChange event is fired when there is still unread data in the requests response buffer.

SOLUTION

When the onFinish handler is called in the xhr receiver read and dispatch any pending messages still in the buffer

CODE REVIEW

@trentgrover-wf
@maxwellpeterson-wf
@dustinlessard-wf
@jayudey-wf
@grantnelson-wf
@nathanevans-wf

});
xo.onFinish.listen((e) {
dispatch(new StatusEvent("chunk", e.status, e.text));
Copy link
Author

Choose a reason for hiding this comment

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

this event doesn't make sense, the XHRObject is the only one who should be emitting chunk events. The receiver should only emits CloseEvents ands MessageEvents. No one would even be listening to this.

@grantnelson-wf
Copy link

+1 Looks good

@evanweible-wf
Copy link
Contributor

+1

1 similar comment
@maxwellpeterson-wf
Copy link
Member

+1

@mackenzieobleness-wf
Copy link

+10 - Dave walked me through reproducing the error; don't see it any more with these changes!

@evanweible-wf
Copy link
Contributor

@jayudey-wf ready for merge

@jayudey-wf jayudey-wf changed the title Read remaining data from request buffer on xhr close CP-1743 Read remaining data from request buffer on xhr close May 10, 2016
@@ -7,4 +7,5 @@ before_install:
- sh -e /etc/init.d/xvfb start
script:
- npm install
- pub run dart_dev test --unit
Copy link
Contributor

Choose a reason for hiding this comment

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

unit tests should be run be default in dart_dev I believe

Copy link
Author

Choose a reason for hiding this comment

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

I thought so too, but didn't see them being run in the CI output for commit f235d0c
https://travis-ci.org/Workiva/sockjs-dart-client/builds/129480139

Copy link
Contributor

Choose a reason for hiding this comment

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

well then disregard my comment, sorry david

Copy link
Contributor

Choose a reason for hiding this comment

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

They might have just gotten swallowed by the integration tests since they take a bit longer. There are 21 integration tests and you added 2 tests, and that CI run had 23 passing tests, so I think they're running.

@evanweible-wf
Copy link
Contributor

Just a couple of small things, tests look good thanks for adding those!

@evanweible-wf
Copy link
Contributor

+1

1 similar comment
@grantnelson-wf
Copy link

+1

@jayudey-wf
Copy link
Contributor

jayudey-wf commented May 11, 2016

QA Resource Approval: +10

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • performed and documented by Mackenzie (this was done prior to the last three commits but those were all for tests and the travis.yaml file)
  • Unit test created/updated
  • All unit tests pass

Merging into master.

@jayudey-wf jayudey-wf merged commit e9a859b into Workiva:master May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants