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.

Sign up for GitHub

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

Jump to bottom

chore: migrate to structuredClone #20885

Merged
merged 3 commits into from
Apr 27, 2023
Merged

chore: migrate to structuredClone #20885

merged 3 commits into from
Apr 27, 2023

Conversation

JamieMagee
Copy link
Contributor

Changes

Migrate from our custom clone to structuredClone

Context

Required as part of #20553

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@JamieMagee JamieMagee requested a review from viceice March 12, 2023 18:26
rarkins
rarkins previously approved these changes Mar 12, 2023
View reviewed changes
@rarkins
Copy link
Collaborator

rarkins commented Mar 12, 2023

Tested on a real repo? :)

viceice
viceice requested changes Mar 12, 2023
View reviewed changes
@viceice
Copy link
Member

viceice commented Mar 12, 2023

also coverage issues

@JamieMagee
Copy link
Contributor Author

Moving this to draft.

@JamieMagee JamieMagee marked this pull request as draft March 12, 2023 21:15
@JamieMagee
Copy link
Contributor Author

The test failures appear to be due to toStrictEqual

@JamieMagee
Copy link
Contributor Author

I'm not sure if this is a bug in jest or not. JSON.parse(JSON.stringify()) and structuredClone() return different results for toStrictEqual. Here's a minimal reproduction and a GitHub Actions run with comparisons.

@viceice
Copy link
Member

viceice commented Mar 13, 2023

strange behavior 🤔

@rarkins
Copy link
Collaborator

rarkins commented Mar 14, 2023

image

Your test case is so simple though. Maybe a jest bug?

@JamieMagee
Copy link
Contributor Author

I opened a bug upstream: jestjs/jest#14011

@viceice
Copy link
Member

viceice commented Mar 16, 2023

can we remove / ignore the strict equal test for now and continue?

maybe add a json stringify compare test instead?

@JamieMagee
Copy link
Contributor Author

Okay, let me scope this PR down a bit and merge what we can.

@JamieMagee JamieMagee force-pushed the chore/structuredClone branch 2 times, most recently from dc4aad6 to bba27dd Compare March 26, 2023 22:44
@JamieMagee
Copy link
Contributor Author

I changed toStrictEqual to toEqual where necessary, because structuredClone doesn't walk to prototype chain1. I think this also causes nock to not match mocked requests. I reintroduced our custom clone for HTTP requests only, and marked our clone method as deprecated, to indicate that it shouldn't be used in new code.

This should allow us to move forward with this PR, and address the issue with nock later.

Footnotes

  1. https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm#things_that_dont_work_with_structured_clone ↩

@JamieMagee JamieMagee marked this pull request as ready for review March 27, 2023 03:24
@JamieMagee JamieMagee requested a review from viceice March 27, 2023 03:26
rarkins
rarkins previously approved these changes Mar 27, 2023
View reviewed changes
viceice
viceice previously approved these changes Mar 27, 2023
View reviewed changes
@viceice
Copy link
Member

viceice commented Mar 27, 2023

coverage failure

@JamieMagee JamieMagee dismissed stale reviews from viceice and rarkins via c8cb28a April 26, 2023 04:31
@JamieMagee JamieMagee force-pushed the chore/structuredClone branch from 0707bdc to c8cb28a Compare April 26, 2023 04:31
@JamieMagee JamieMagee force-pushed the chore/structuredClone branch from c8cb28a to 9f734f3 Compare April 26, 2023 04:48
rarkins
rarkins previously approved these changes Apr 26, 2023
View reviewed changes
viceice
viceice previously approved these changes Apr 26, 2023
View reviewed changes
@JamieMagee JamieMagee dismissed stale reviews from viceice and rarkins via 7c6b31b April 27, 2023 04:18
@JamieMagee JamieMagee enabled auto-merge April 27, 2023 04:18
@JamieMagee JamieMagee added this pull request to the merge queue Apr 27, 2023
Merged via the queue into main with commit a6dfa70 Apr 27, 2023
@JamieMagee JamieMagee deleted the chore/structuredClone branch April 27, 2023 05:34
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 35.62.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot mentioned this pull request May 2, 2023
Merged
mjunker pushed a commit to mjunker/renovate that referenced this pull request May 17, 2023
@JamieMagee @rarkins @mjunker
chore: migrate to structuredClone (renovatebot#20885)
ff28ca4 
Co-authored-by: Rhys Arkins <rhys@arkins.net>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rarkins rarkins rarkins approved these changes

@viceice viceice viceice left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JamieMagee @rarkins @viceice @renovate-release