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: subscriptions with wildcard pattern matching #1152

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Aug 5, 2024

User description

Nasr helped us by giving us this quick fix. We can remove this when we switch to dojo-1.0-alpha.5


PR Type

Bug fix


Description

  • Reordered imports in client/src/dojo/setup.ts for better organization.
  • Fixed the subscriptions setup by modifying the getSyncEntities call to include wildcard pattern matching.
  • Added a configuration object to the getSyncEntities function to support pattern matching with VariableLen.

Changes walkthrough 📝

Relevant files
Bug fix
setup.ts
Fix wildcard pattern matching in subscriptions setup         

client/src/dojo/setup.ts

  • Reordered imports for better organization.
  • Modified getSyncEntities call to include wildcard pattern matching.
  • Added a configuration object for pattern matching in getSyncEntities.
  • +17/-4   

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

    Copy link

    vercel bot commented Aug 5, 2024

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 5, 2024 10:39pm

    Copy link

    github-actions bot commented Aug 5, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Clarity
    The use of as any in line 19 can lead to type safety issues and makes the code less maintainable and error-prone. Consider providing a proper type or interface instead of using type assertions.

    Hardcoded Value
    The hardcoded value 1000 in line 29 might need clarification or extraction to a constant with a descriptive name to improve code readability and maintainability.

    Copy link

    github-actions bot commented Aug 5, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Improve type safety by replacing any with a specific type

    Replace the use of as any with a more specific type to ensure type safety. Using any
    can lead to runtime errors that are hard to debug. If the type of
    network.contractComponents is not known, it might be beneficial to define an
    interface or use an existing type that closely matches the expected structure.

    client/src/dojo/setup.ts [19]

    -network.contractComponents as any
    +network.contractComponents as SpecificType
     
    Suggestion importance[1-10]: 8

    Why: Replacing any with a specific type enhances type safety and reduces the risk of runtime errors. This is a significant improvement for maintainability and debugging.

    8
    Replace magic number with a named constant to clarify its purpose

    Replace the magic number 1000 with a named constant to clarify its purpose and
    enhance code readability.

    client/src/dojo/setup.ts [29]

    -1000,
    +MAX_SYNC_TIMEOUT,
     
    Suggestion importance[1-10]: 7

    Why: Replacing the magic number with a named constant enhances code readability and maintainability by making the code more self-explanatory.

    7
    Possible issue
    Validate arrays to ensure they contain elements before use

    Consider validating the keys and models arrays to ensure they are not empty before
    making the network request. Passing empty arrays might lead to unnecessary network
    calls or unexpected behaviors.

    client/src/dojo/setup.ts [22-25]

    -keys: [],
    -models: []
    +keys: validatedKeys,
    +models: validatedModels
     
    Suggestion importance[1-10]: 7

    Why: Validating the keys and models arrays before use can prevent unnecessary network calls and unexpected behavior, improving the robustness of the code.

    7
    Maintainability
    Improve readability by extracting complex object into a constant

    Extract the complex object used in getSyncEntities into a constant outside the
    function. This will improve code readability and maintainability by separating the
    configuration from the function logic.

    client/src/dojo/setup.ts [17-30]

    +const entityConfig = {
    +  Keys: {
    +    keys: [],
    +    pattern_matching: "VariableLen",
    +    models: [],
    +  },
    +};
     const sync = await getSyncEntities(
         network.toriiClient,
    -    network.contractComponents as any,
    -    [
    -      {
    -        Keys: {
    -          keys: [],
    -          pattern_matching: "VariableLen",
    -          models: [],
    -        },
    -      },
    -    ],
    +    network.contractComponents as SpecificType,
    +    [entityConfig],
         1000,
       );
     
    Suggestion importance[1-10]: 6

    Why: Extracting the complex object into a constant improves code readability and maintainability by separating configuration from logic, though it is a minor improvement.

    6

    Copy link
    Contributor

    @mentatbot mentatbot bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This pull request introduces a temporary fix for subscriptions with wildcard pattern matching. The changes look good and should improve the functionality of the getSyncEntities call. However, it's important to remember that this is a temporary solution and should be revisited when upgrading to dojo-1.0-alpha.5. The code is clean and well-formatted, which is appreciated. Consider adding a TODO comment to remind about the future removal of this fix, and possibly review if the 'VariableLen' pattern matching is the most efficient for your use case.

    Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

    @@ -14,7 +14,20 @@ export async function setup({ ...config }: DojoConfig) {
    const updates = await createUpdates();

    // fetch all existing entities from torii
    const sync = await getSyncEntities(network.toriiClient, network.contractComponents as any, [], 1000);
    const sync = await getSyncEntities(
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This change improves the getSyncEntities call by adding a specific configuration for key pattern matching. It's a good temporary fix, but as mentioned in the PR description, it should be removed when upgrading to dojo-1.0-alpha.5. Consider adding a TODO comment to remind about this future change.

    network.contractComponents as any,
    [
    {
    Keys: {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    The pattern_matching: "VariableLen" setting allows for more flexible entity querying. However, it might be worth considering if this is the most efficient approach for your specific use case. If you know the exact structure of your entities, a more specific pattern might be more performant.

    @aymericdelab aymericdelab self-requested a review August 6, 2024 13:20
    @aymericdelab aymericdelab merged commit fa136e7 into main Aug 6, 2024
    21 checks passed
    @aymericdelab aymericdelab deleted the fix/subscriptions branch August 6, 2024 13:20
    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