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

Fix check for support of structured clone #10

Merged
merged 3 commits into from
Oct 29, 2019
Merged

Conversation

jsandoe
Copy link
Member

@jsandoe jsandoe commented Oct 16, 2019

The structured clone check was initially successful, but failed to remove its handler.
On subsequent activity, the check for a Safari 5.1 defect would
reset the structured clone supported flag.

Check was initially successful, but failed to remove its handler.
On subsequent activity, the check for a Safari 5.1 defect would
reset the structured clone supported flag.
@jsandoe jsandoe requested a review from scytacki October 16, 2019 16:16
Copy link
Member

@scytacki scytacki left a comment

Choose a reason for hiding this comment

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

Did you check if adding the handler is needed at all?
Can we just call postMessage and check for an error?

@jsandoe
Copy link
Member Author

jsandoe commented Oct 16, 2019

Works fine without the handler and is cleaner. Unit tests run. Runs fine in CODAP. I don't quite know how to exercise the failure case, though.

Now, the checker sends an illegal message to itself and expects an exception
to be thrown. It examines the result for keywords.
@scytacki
Copy link
Member

I've tried to decipher the caniuse details more. It seems too hard to figure out how to test the failure case. Basically we'd need Firefox version 5 or earlier I think.

Since we are checking based on an exception and the whole bit of Safari code was because Safari actually doing the structure clone, but wasn't throwing the exception. And since the throwing of that exception is not something that is tested by the caniuse folks. It is really hard to know what we should actually test.

Sorry for such a run around on this, but I now thing we should just drop the structured clone check all together. No one has responded on dev slack channel. As long as we are updating this code and we are going to have to do some testing of multiple browsers around it, we might as well go all of the way.

@jsandoe
Copy link
Member Author

jsandoe commented Oct 16, 2019

I found this: https://diff-dot-wptdashboard-staging.appspot.com/results/workers/semantics/structured-clone?label=experimental&label=master&aligned

Suggests current Edge doesn't support Structured Clone, but I haven't yet investigated further...

@scytacki
Copy link
Member

scytacki commented Oct 29, 2019

I've reviewed: Modernizr/Modernizr#388
and
Modernizr/Modernizr#1250
The test we are using came from the issue above. That approach is now replaced by a different test of whether toString is called on the object passed to postMessage. However the create of that tests seems to imply using the original check for a CLONE_ERR might be better. But that never seemed to be resolved and then 4 years later (2018) the toString approach was merged in.

The tests linked above by @jsandoe provide a comprehensive view of structured clone support on the latest builds of the major desktop browsers. They generally seem to support it quite well. Unfortunately I can't tell if there is a way to use this site to find out how well and how long the stable versions of browsers have supported it.

Anyhow I think we might as well stick with this PR as is and see how it works. At worst it should use strings on old versions of safari when it should be using structured clone. And previously all browsers were using strings because of the issue in the safari code.

@scytacki scytacki merged commit 8680f1e into master Oct 29, 2019
@scytacki scytacki deleted the fix-structured-clone branch October 29, 2019 01:36
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.

2 participants