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

Ensure target and currentTarget properties are set on events #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andybluntish
Copy link

We had a problem where a library was making XHRs and expecting event.currentTarget to be set to the XHR object in a readystatechange handler.

  1. Set currentTarget on Events
    When listening for events (e.g. readystatechange) some libraries expect event.currentTarget to be set to the XHR, as well as event.target. Since the readystatechange event doesn't bubble, the target and currentTarget will always be the same.

  2. Set the target for readystatechange events
    Ensure we can access the XHR via target/currentTarget when listening to
    the readystatechange event.

Copy link
Contributor

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

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

This change requires corresponding tests

@andybluntish
Copy link
Author

Thank you. I added some tests, please let me know if they're not sufficient.

Also, running the tests updated the built fake_xml_http_request.js file, but not the corresponding source map file. Is there a build step or a preferred way to update the source map that I'm not seeing?

@timiyay
Copy link

timiyay commented Jan 8, 2019

I'm picking this issue up, so I can move our company off our forks of FakeXMLHttpRequest, pretender, and mirage.

If we can get this issue fixed up, and bump all the versions so that the fix is available to mirage, then we won't need our forks.

@stefanpenner If you get a moment, are you able to review this PR? I can make any necessary changes from the review.

@BarryThePenguin
Copy link

Looking at Comparison of Event Targets on MDN

event.target

The DOM element on the lefthand side of the call that triggered this event, eg: element.dispatchEvent(event)

event.currentTarget

The EventTarget whose EventListeners are currently being processed. As the event capturing and bubbling occurs this value changes.

sinonjs also does a similar thing in nise

https://github.com/sinonjs/nise/blob/e2dc539f07ceda87e7434362e12d4446a917e894/lib/fake-xhr/index.js#L258-L262

I think this may even be related to #28?

Andy Stanford-Bluntish added 2 commits January 9, 2020 17:06
When listening for events (e.g. readystatechange) some libraries expect
`event.currentTarget` to be set to the XHR, as well as `event.target`.
Since the `readystatechange` event doesn't bubble, the `target` and
`currentTarget` will always be the same.
Set the event target for the progress event via the constructor, for
consistency with other events. Also ensures currentTarget is set at the
same time.
@asppsa
Copy link

asppsa commented Jan 9, 2020

I've rebased this PR onto the current master and compacted the commits down into two: one for readystatechange events, and one for progress events. @xg-wang, we're happy to make whatever further changes are necessary to get this PR accepted.

@kaizau
Copy link

kaizau commented May 6, 2020

@stefanpenner Is there anything else preventing this PR from getting accepted?

This gets FakeXMLHttpRequest to behave more realistically, in addition to unblocking those of us who are relying on forks.

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