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

fix: schema.All INVALID denormalize case when class name mangling for prod builds #2954

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

ntucker
Copy link
Collaborator

@ntucker ntucker commented Feb 20, 2024

Motivation

  • Do not rely on 'name' detection as production builds often rename functions and class names
  • Remove tight coupling with schema definitions (in @data-client/endpoint) from denormalize() algorithm (in @data-client/normalizr)

Solution

Fix schema.All denormalize INVALID case should also work when class name mangling is performed in production builds

  • unvisit() always returns undefined with undefined as input.
    • Without this one-off complexity, we are able to simplify the logic in unvisit greatly
  • All returns INVALID from infer() to invalidate what was previously a special case in unvisit() (when there is no table entry for the given entity)

New unvisit:

function unvisit(input: any, schema: any): any {
  if (!schema) return input;

  if (input === null || input === undefined) {
    return input;
  }

  if (typeof schema.denormalize !== 'function') {
    // deserialize fields (like Temporal.Instant)
    if (typeof schema === 'function') {
      return schema(input);
    }

    // shorthand for object, array
    if (typeof schema === 'object') {
      const method =
        Array.isArray(schema) ? arrayDenormalize : objectDenormalize;
      return method(schema, input, args, unvisit);
    }
  } else {
    if (isEntity(schema)) {
      return unvisitEntity(input, schema, args, unvisit, getEntity, cache);
    }

    return schema.denormalize(input, args, unvisit);
  }

  return input;
}

Copy link

changeset-bot bot commented Feb 20, 2024

🦋 Changeset detected

Latest commit: 7c20d81

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages
Name Type
@data-client/normalizr Major
@data-client/endpoint Major
@data-client/react Major
@data-client/core Major
@data-client/rest Major
@data-client/hooks Patch
example-benchmark Patch
normalizr-github-example Patch
normalizr-redux-example Patch
normalizr-relationships Patch
rdc-website Patch
@data-client/img Major
@data-client/redux Major
@data-client/ssr Major
@data-client/test Major
@data-client/graphql Major
coinbase-lite Patch

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

@ntucker ntucker force-pushed the all-name-mangling branch 3 times, most recently from 082d0ba to 00d7f3e Compare February 20, 2024 13:11
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (72ecb71) 98.69% compared to head (7c20d81) 98.69%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2954      +/-   ##
==========================================
- Coverage   98.69%   98.69%   -0.01%     
==========================================
  Files         116      116              
  Lines        2072     2068       -4     
  Branches      416      413       -3     
==========================================
- Hits         2045     2041       -4     
  Misses         16       16              
  Partials       11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ntucker ntucker merged commit eae6fd9 into master Feb 20, 2024
23 checks passed
@ntucker ntucker deleted the all-name-mangling branch February 20, 2024 13:29
@github-actions github-actions bot mentioned this pull request Feb 20, 2024
ntucker added a commit that referenced this pull request Feb 20, 2024
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.

1 participant