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

Ignore TypedArrays when freezing object #8813

Merged
merged 4 commits into from
Sep 20, 2021
Merged

Conversation

geekuillaume
Copy link
Contributor

@geekuillaume geekuillaume commented Sep 19, 2021

Checklist:

  • Make sure all of the significant new logic is covered by tests

This PR fixes #6813 by ignored TypedArrays when freezing objects.

@apollo-cla
Copy link

@geekuillaume: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@brainkim
Copy link
Contributor

@benjamn This looks good to me! Though perhaps you have a more comprehensive solution including more edge cases.

@benjamn benjamn added this to the v3.4.x patch releases milestone Sep 20, 2021
@geekuillaume
Copy link
Contributor Author

Like said in my previous comment #6813 (comment), we could just ignore errors when applying Object.freeze to make this future-proof. On one hand it's maybe a bad idea because it could hide real errors, on the other hand this code is stripped in production so the consequences are limited.

@benjamn
Copy link
Member

benjamn commented Sep 20, 2021

@geekuillaume @brainkim I agree we should err on the side of silencing all possible TypeErrors, since there are a handful of other types that cause a TypeError when passed to Object.freeze (and more could be added to the language or to host environments like Node.js in the future). See 87f604e for my take!

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started @geekuillaume! Since I assigned #6813 to myself, you technically did (most of) my job for me—much appreciated.

@benjamn benjamn merged commit 4b9a77c into apollographql:main Sep 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache and Typed arrays
4 participants