search

TrueCar / react-launch-darkly

Simple component helpers to support LaunchDarkly in your React app.
MIT License
76 stars 20 forks source link

XMLHttpRequest failure: synchronous XHR request during page dismissal #111

Closed Dnguye92 closed 4 years ago

Dnguye92 commented 4 years ago

Chrome no longer allows synchronous XHR calls thus throwing the error above.

One of the possible workarounds mentioned in this issue is: To reduce—but not eliminate—the chance of these warnings happening, you can set a lower flushInterval in your configuration, such as 1000 (one second). That will make it somewhat less likely that there are events still waiting to be sent when the page is closed. The tradeoff is that the browser will be making more frequent HTTP requests to LaunchDarkly.

But there doesn't seem anywhere in the react-launch-darkly package to take care of this.

Thoughts?

sethbattin commented 4 years ago

Looking at this comment on launchdarkly's issue, it seems like they have already resolved the problem in their library. We could update our usage right here https://github.com/TrueCar/react-launch-darkly/blob/master/package.json#L46

Is that a thing you're willing to submit a PR to accomplish, @Dnguye92 ?

Dnguye92 commented 4 years ago

@sethbattin sure thing!

sethbattin commented 4 years ago

@Dnguye92 so there's good news and bad news. The good news is, although I botched the automatic npm release by tagging without updating the version in the package file, there is now a release you can try on the releases page https://github.com/TrueCar/react-launch-darkly/releases/tag/v4.1.0

The bad news is that is exercise was probably wasted effort :( ldclient-js is actually a peer dependency of this library (also a dev dependency for local testing). That means you can install whatever version you want in your project, and we didn't need to update anything here. I'm sorry for forgetting this at the time you posted your bug.

The silver lining is that in the future folks who install the devdep version won't see the error. But in general, if you are using launch darkly's analytics which is separate from their flag system, I would recommend updating to their latest library regardless.

Sorry again for the rigmarole, but at least you can fix your error source. 🙃

jzaefferer commented 4 years ago

@sethbattin looks like ldclient-js is deprecated and hasn't been updated in over a year. The replacement is launchdarkly-js-client-sdk. I could install that, but I don't think that would fulfill the peer-dependency of react-launch-darkly.

Maybe update to 4.2.0 with a proper npm release, replacing ldclient-js with launchdarkly-js-client-sdk?

sethbattin commented 4 years ago

@jzaefferer I like that idea a lot. I think that in terms of how annoying it would be to juggle a switch from one peer dep to another, we should probably call it a major version number increase. On the plus side, we have another open PR (typescript conversion #110 and #114) that also warrants a major version. 5.0.0 could get two whole changes in one release! 🙃

Would you mind typing up another issue with the proposal? Feel free to add the patch or not; I have no sense of the level of effort required to switch from using ldclient-js to launchdarkly-js-client-sdk.

sethbattin commented 4 years ago

@jzaefferer I went ahead with the patch already in #117; i do intend to deploy it among other changes in a 5.0 branch. Thank you again for the great suggestion!

Between the peer dep workaround and the side topic of releasing these changes, I'm going to close this one.