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

Copy Symbols as well in copyOwnPropsIfNotPresent #4505

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hetch3t
Copy link

@Hetch3t Hetch3t commented Jun 4, 2022

Hi!

I had issues with mocking lib - we use ObjectId from Mongo as an ID scalar type, so after introducing mocking to our schema it started to fail. After hefty amount of time debugging it turned out that ObjectId is implemented in such a way that is is stored as buffer as Symbol(id). Function that is being used in @graphql-tools/mock uses Object.getOwnPropertyNames which doesn't list Symbols. The solution I've implemented you can see in this PR.

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2022

⚠️ No Changeset found

Latest commit: 5539ec0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jun 4, 2022

@Hetch3t is attempting to deploy a commit to the The Guild Team on Vercel.

A member of the Team first needs to authorize it.

@ardatan
Copy link
Owner

ardatan commented Jun 5, 2022

Thanks for the PR! Would you add some unit tests to prevent future regressions and changeset(using yarn changeset)?

@ardatan ardatan force-pushed the master branch 2 times, most recently from 4946aeb to 4be9073 Compare July 20, 2022 20:01
@theguild-bot theguild-bot mentioned this pull request Apr 23, 2023
@theguild-bot theguild-bot mentioned this pull request Oct 26, 2023
@bkalbs
Copy link

bkalbs commented Nov 28, 2023

We just encountered this same issue when attempting to mock the MongoID scalar type (started getting error "Cannot create Buffer from undefined"), which led me to this PR. I quick/dirty tested the proposed changes here and they seem to work wonderfully🥇!

Other than needing tests, is there anything else that is blocking this fix from making it into a release? At over a year old, is this PR still valid? I see that it's listed in the project roadmap, but just wanted to give it a bump.

@Urigo
Copy link
Collaborator

Urigo commented Dec 2, 2023

@bkalbs-wm would you be able to contribute a test for this?

@bkalbs
Copy link

bkalbs commented Dec 6, 2023

@Urigo i can give it a go once I get some breathing room from this work deadline i am on. would like to help however i can. I can open up a new PR once ready.

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.

4 participants