-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
unify algorithms for masking and migration #12139
Conversation
🦋 Changeset detectedLatest commit: dd9004f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Docs Preview ReadyNo new or changed pages found. |
commit: |
size-limit report 📦
|
e809281
to
c2c596e
Compare
482f59d
to
bdc3e24
Compare
This should not have any immediate effect, but will prevent additional memory leaks in case the `context` object leaks anywhere.
definition.selectionSet, | ||
context | ||
); | ||
const [masked, changed] = disableWarningsSlot.withValue(true, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things I see and wonder why I didn't think of it sooner 😆. Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to push a few minor updates before merging, but otherwise this looks great! Looking forward to getting this in. Thanks for the assist 🙂
src/core/masking.ts
Outdated
path: string | undefined, | ||
migration: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path: string | undefined, | |
migration: boolean | |
migration: boolean, | |
path?: string | undefined, |
I think it might make sense to flip these around, which means we can keep path
optional. Since migration
is required anyways, might as well reduce a required argument in those areas we pass undefined
anyways. Should reduce a touch of bundle size.
} | ||
|
||
function addAccessorWarning( | ||
data: Record<string, any>, | ||
function getAccessorWarningDescriptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this refactor!
Probably petty, but now that we aren't passing data
as the first argument, can we swap the value
and fieldName
arguments? Right now it feels inverse to me and my brain breaks 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch around as you like :) I just wanted to keep data out to prevent potential memory leaks, and started writing to value
so we don't keep a reference to the original value
around forever
No description provided.