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

Cache and Typed arrays #6813

Closed
Banou26 opened this issue Aug 10, 2020 · 9 comments · Fixed by #8813
Closed

Cache and Typed arrays #6813

Banou26 opened this issue Aug 10, 2020 · 9 comments · Fixed by #8813
Assignees

Comments

@Banou26
Copy link
Contributor

Banou26 commented Aug 10, 2020

Intended outcome:
When trying to pass TypedArrays as a local-only field it should work

Actual outcome:
Apollo throws Cannot freeze array buffer views with elements

How to reproduce the issue:
Pass a non empty TypedArray to a local-only field.

@benjamn
Copy link
Member

benjamn commented Aug 10, 2020

Hmm, since the freezing happens only in development, and is intended to encourage immutable/non-destructive updates rather than strictly enforcing them, maybe it should be best-effort, as the name maybeDeepFreeze suggests.

I admit I was not aware of any object types that throw when frozen with Object.freeze, but since that's apparently a real possibility for TypedArray, I'm open to catching/silencing those exceptions, and just leaving such objects unfrozen.

Out of curiosity, are you modifying these TypedArrays in place, or are they immutable? Are they just scalar values (field name without selection set), or do you treat them as GraphQL data?

@Banou26
Copy link
Contributor Author

Banou26 commented Aug 10, 2020

I'm following the immutability mindset & replacing the entire TypedArray if i ever need to change its value(in my case this will practically never happen), and yes, I want to use them as scalar values without any selection.

Though, if you are using a nodejs Buffer polyfill, the Apollo WebExtension representation will be {"type": "Buffer", "data": [...]}

PackageBundle:{"version":"0.0.1","package":{"id":"5f316cb6fd847fcc5307f611"}}
archive: {"type":"Buffer","data":[4,0,0,0,148,0,0,0,144,0,0,0,139,0,0,0,123,34,102,105,108,101,115,34,58,123,34,109,97,110,105,102,101,115,116,46,106,115,111,110,34,58,123,34,115,105,122,101,34,58,49,50,52,44,34,111,102,102,115,101,116,34,58,34,48,34,125,44,34,105,110,100,101,1,...]}

And if you are simply passing a native TypedArray, the representation will be an object's representation of an array.

PackageBundle:{"version":"0.0.1","package":{"id":"5f316cb6fd847fcc5307f611"}}
archive: {"0":4,"1":0,"2":0,"3":0,"4":148,"5":0,"6":0,"7":0,"8":144,"9":0,"10":0,"11":0,"12":139,"13":0,"14":0,"15":0,"16":123,"17":34,"18":102,"19":105,"20":108,"21":101,"22":115,"23":34,"24":58,"25":123,"26":34,"27":109,"28":97,"29":110,"30":105,"31":1

I'm not sure what will be the actual returned value since this throws, but if this is the returned data I don't think that this would be an expected result.

@kevin-lindsay-1
Copy link

Getting the same issue only in development.

I generally use custom scalars for the sake of notation for these browser-side types and use immutability throughout.

@hwillson
Copy link
Member

hwillson commented May 5, 2021

Let us know if this is still a concern with @apollo/client@latest - thanks!

@hwillson hwillson closed this as completed May 5, 2021
@Banou26
Copy link
Contributor Author

Banou26 commented May 6, 2021

This is still an issue under ^3.3.16 which is current latest
Here is a runnable repro of exactly what i described to reproduce the error in the original comment:

Set a typed array into a reactive variable and query it.

https://github.com/Banou26/apollo-client-bug-6813

As a side note, closing out issues that were left abandoned for months and are still valid issues isn't really respectful for the time we spent describing/making repros for issues that we reported. I'm still gonna double check all my active issues you closed. It's certainly not pleasant having to report the same issue multiple times.

@brainkim
Copy link
Contributor

brainkim commented May 6, 2021

As a side note, closing out issues that were left abandoned for months and are still valid issues isn't really respectful for the time we spent describing/making repros for issues that we reported. I'm still gonna double check all my active issues you closed. It's certainly not pleasant having to report the same issue multiple times.

Sorry about that! We’ve been closing issues because there were so many which were referencing outdated versions and investigating each of them would be cost prohibitive. Of course, this is our fault for not tracking issues and closing them as they’re fixed, but we’ve found that closing issues was a great way to find out which issues people still cared about. I’ll make sure any of your issues are looked at more carefully in the future.

@brainkim brainkim reopened this May 6, 2021
@hwillson
Copy link
Member

hwillson commented May 6, 2021

Thanks @Banou26 - as @brainkim mentioned, our intent is to get our issue backlog under control, so we can start being more effective at staying on top of and resolving issues. To get the hundreds of old issues under control, we have to make certain assumptions. Asking people to verify the issue with @apollo/client@latest and closing the issue really does help clear out the backlog, as there are hundreds of issues that are already fixed, and just haven't been closed. We're keeping an eye on comments on closed issues though, and as long as people can provide a reproduction (or update an existing reproduction), we're re-opening them. Again though, we're not trying to be disrespectful here - quite the opposite. Instead of letting an ~850 open issue count stagnate, meaning the core team is only really staying on top of new issues, we're going to clean up the entire backlog, and stay on top of all of them. This will lead to faster turn around times on issues.

@p-j
Copy link

p-j commented Sep 8, 2021

I encountered this error yesterday as well. Two things that made it difficult to work with:

  • it throws when really this should be a warning/error logged in the console with explanations and links to documentation/best practices since this behavior isn't blocking in production
  • it doesn't tell me what's the issue: what object/array caused that error (neither the value, nor the type or its key are given).

in the end it was an issue with some shared code between frontend & backend that was producing ObjectId instead of their string representation; ObjectId internally uses an Uint8Array that was crashing the maybeDeepFreeze function.

@geekuillaume
Copy link
Contributor

I submitted a PR to fix this: #8813. This simply ignores TypedArrays when freezing. We could also just wrap Object.freeze in a try/catch statement and trigger a warning on error to make this future-proof in case some other objects that are not freezable are added in the future.

@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 a pull request may close this issue.

7 participants