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

Dev 3831 experience js experience mapper wont default a null or missing variants array correctly #54

Conversation

BraunreutherA
Copy link
Contributor

@BraunreutherA BraunreutherA commented Jun 13, 2024

User description

This PR fixes a bug which made it possible to put variants as null or undefined into the isExperienceEntry as valid object. As the variants didn't get defaulted to an empty array, the ExperienceMapper.mapExperience function failed.


PR Type

Bug fix, Tests


Description

  • Added tests to ensure null and invalid variants are not accepted in ExperienceEntry.
  • Added tests to default missing or null variants to an empty array in Experiment and Experience.
  • Added test to ensure the type of variants is preserved in ExperienceMapper.
  • Fixed the parse and safeParse functions in Experience and Experiment to correctly handle variants.

Changes walkthrough 📝

Relevant files
Tests
ExperienceEntry.spec.ts
Add tests for invalid and null variants in ExperienceEntry

packages/utils/contentful/src/types/ExperienceEntry.spec.ts

  • Added tests to ensure null and invalid variants are not accepted.
  • +33/-0   
    ExperienceMapper.spec.ts
    Add test to preserve variant types in ExperienceMapper     

    packages/utils/javascript/src/lib/ExperienceMapper.spec.ts

    • Added test to ensure the type of variants is preserved.
    +17/-0   
    Experiement.spec.ts
    Add tests for defaulting and validating variants in Experiment

    packages/utils/javascript/src/types/Experiement.spec.ts

  • Added tests to default missing or null variants to an empty array.
  • Added test to validate the variants array.
  • +43/-0   
    Experience.spec.ts
    Add tests for defaulting and validating variants in Experience

    packages/utils/javascript/src/types/Experience.spec.ts

  • Added tests to default missing or null variants to an empty array.
  • Added test to validate the variants array.
  • +43/-0   
    Bug fix
    Experience.ts
    Fix variant handling in Experience parse functions             

    packages/utils/javascript/src/types/Experience.ts

  • Fixed the parse and safeParse functions to correctly handle variants.
  • +2/-2     
    Experiment.ts
    Fix variant handling in Experiment parse functions             

    packages/utils/javascript/src/types/Experiment.ts

  • Fixed the parse and safeParse functions to correctly handle variants.
  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    3

    🧪 Relevant tests

    Yes

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The use of TypeScript's @ts-expect-error comments in tests could mask other potential type errors that are not intended to be tested. Consider using more precise type assertions or handling.

    Code Duplication:
    Similar test scenarios and expectations are repeated across multiple test files (ExperienceEntry.spec.ts, Experiment.spec.ts, and Experience.spec.ts). Consider abstracting common logic or expectations to reduce redundancy and improve maintainability.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Type safety
    Use a type-safe approach to ensure output.variants is of type T[]

    Instead of casting output.variants to T[], consider using a type-safe approach to ensure
    output.variants is of type T[]. This can be done by validating the type during the parsing
    process.

    packages/utils/javascript/src/types/Experience.ts [67]

    -variants: output.variants as T[],
    +variants: Array.isArray(output.variants) ? output.variants as T[] : [],
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion to use a type-safe approach instead of a simple cast is crucial for preventing runtime errors and ensuring data integrity. This is a significant improvement in the code's robustness.

    8
    Use a type-safe approach to ensure output.data.variants is of type T[]

    Instead of casting output.data.variants to T[], consider using a type-safe approach to
    ensure output.data.variants is of type T[]. This can be done by validating the type during
    the parsing process.

    packages/utils/javascript/src/types/Experiment.ts [48]

    -variants: output.data.variants as T[],
    +variants: Array.isArray(output.data.variants) ? output.data.variants as T[] : [],
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Similar to the previous suggestion, ensuring type safety for output.data.variants is essential for maintaining the integrity and reliability of the data structure, making this a valuable improvement.

    8
    Best practice
    Update type definitions to handle null values instead of suppressing TypeScript errors

    Instead of using @ts-expect-error to suppress TypeScript errors, consider updating the
    type definitions to handle null values for nt_variants. This will make the code more
    robust and maintainable.

    packages/utils/contentful/src/types/ExperienceEntry.spec.ts [21-23]

    -// eslint-disable-next-line @typescript-eslint/ban-ts-comment
    -// @ts-expect-error
    -nt_variants: null,
    +nt_variants: null as unknown as SomeType[],
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies a potential improvement in handling null values for nt_variants by updating type definitions instead of suppressing errors. This approach enhances maintainability and robustness.

    7
    Add a type guard function to check for the hidden property in the variant object

    Add a type guard function to check if the variant object has the hidden property, which
    will make the code more readable and type-safe.

    packages/utils/javascript/src/lib/ExperienceMapper.spec.ts [94-96]

    -if (!('hidden' in variant)) {
    -  // Yeay! It correctly inferred the type of the property "foo" on the variant
    +const hasHiddenProperty = (variant: any): variant is { hidden: boolean } => 'hidden' in variant;
    +if (!hasHiddenProperty(variant)) {
       expect(variant.foo).toBe('bar');
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a type guard function as suggested would improve readability and type safety. However, the improvement is relatively minor and specific to this instance.

    6

    @BraunreutherA BraunreutherA merged commit c712180 into main Jun 13, 2024
    5 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants