search

usermaven / usermaven-js

Usermaven provides instant actionable analytics to grow your SaaS business.
MIT License
1 stars 2 forks source link

Keepalive: true to send data during unload events #98

Closed seeratawan01 closed 1 month ago

seeratawan01 commented 1 month ago

PR Type

enhancement, bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
usermaven.ts
Add keepalive option to fetch request for unload events   
packages/javascript-sdk/src/usermaven.ts
  • Added keepalive: true to the fetch request options.
  • Ensures data is sent during unload events.
    		•  +1/-0     

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

    github-actions[bot] commented 1 month ago

    PR Reviewer Guide 🔍

    🏅 Score: 92
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review
    Code feedback:
    relevant filepackages/javascript-sdk/src/usermaven.ts
    suggestion       Consider adding error handling specifically for the `keepalive` option in the fetch request. Since `keepalive` requests have limitations like size constraints and no CORS redirects, it's important to handle cases where the request might fail due to these limitations. This can be done by checking the response status and logging an appropriate error message. [important]
    relevant linekeepalive: true,
    github-actions[bot] commented 1 month ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Conditionally set the keepalive property to enhance compatibility and control ___ **Ensure that the keepalive option is conditionally set based on the environment or
    configuration, as it might not be supported in all browsers or might not be
    necessary in all cases. This can be done by adding a conditional check around the
    keepalive property.** [packages/javascript-sdk/src/usermaven.ts [494]](https://github.com/usermaven/usermaven-js/pull/98/files#diff-a9e765a7bfd2dfbeba110d6b5d12e58d21090506ae52b206581809fae414ebfeR494-R494) ```diff -keepalive: true, +keepalive: shouldUseKeepalive ? true : undefined, ```
    Suggestion importance[1-10]: 8 Why: The suggestion to conditionally set the `keepalive` property is valid, as it addresses potential compatibility issues with different browsers and environments. This change enhances the robustness of the code by allowing more control over when the `keepalive` option is used.
    		8