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

[PLA-1785] remove init promise #117

Merged
merged 1 commit into from
May 31, 2024
Merged

[PLA-1785] remove init promise #117

merged 1 commit into from
May 31, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented May 31, 2024

PR Type

Bug fix, Enhancement


Description

  • Changed initialization to await appStore.init() in multiple components.
  • Removed usage of initPromise and clearInitPromise from various components.
  • Refactored init method in store/index.ts to simplify initialization logic.

Changes walkthrough 📝

Relevant files
Enhancement
3 files
App.vue
Await `appStore.init()` during initialization.                     

resources/js/components/App.vue

  • Changed initialization to await appStore.init().
+2/-2     
Login.vue
Await `appStore.init()` during login.                                       

resources/js/components/pages/auth/Login.vue

  • Changed initialization to await appStore.init().
+1/-3     
index.ts
Refactored `init` method and removed `initPromise`.           

resources/js/store/index.ts

  • Refactored init method to remove initPromise.
  • Simplified initialization logic.
  • +45/-49 
    Bug fix
    10 files
    BeamsList.vue
    Removed `initPromise` and `clearInitPromise` from BeamsList.

    resources/js/components/beam/BeamsList.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-2     
    ClaimsList.vue
    Removed `initPromise` and `clearInitPromise` from ClaimsList.

    resources/js/components/beam/ClaimsList.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-3     
    BidsList.vue
    Removed `initPromise` and `clearInitPromise` from BidsList.

    resources/js/components/marketplace/BidsList.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-2     
    ListingsList.vue
    Removed `initPromise` and `clearInitPromise` from ListingsList.

    resources/js/components/marketplace/ListingsList.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-2     
    SalesList.vue
    Removed `initPromise` and `clearInitPromise` from SalesList.

    resources/js/components/marketplace/SalesList.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-2     
    Collections.vue
    Removed `initPromise` and `clearInitPromise` from Collections.

    resources/js/components/pages/Collections.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-2     
    FuelTanks.vue
    Removed `initPromise` and `clearInitPromise` from FuelTanks.

    resources/js/components/pages/FuelTanks.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-3     
    Tokens.vue
    Removed `initPromise` and `clearInitPromise` from Tokens.

    resources/js/components/pages/Tokens.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-3     
    Transactions.vue
    Removed `initPromise` and `clearInitPromise` from Transactions.

    resources/js/components/pages/Transactions.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-3     
    Wallets.vue
    Removed `initPromise` and `clearInitPromise` from Wallets.

    resources/js/components/pages/Wallets.vue

    • Removed usage of initPromise and clearInitPromise.
    +0/-3     

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

    @zlayine zlayine requested a review from enjinabner May 31, 2024 05:55
    @zlayine zlayine self-assigned this May 31, 2024
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels May 31, 2024
    Copy link

    PR Description updated to latest commit (d9dc4ae)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and changes in the initialization logic across various components. The changes are not overly complex but require careful review to ensure that the new asynchronous behavior is correctly implemented without introducing race conditions or other async-related bugs.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The removal of initPromise and its clearing method could lead to race conditions if not all dependent components are properly updated to handle the new async init method.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileresources/js/components/App.vue
    suggestion      

    Consider handling potential exceptions from appStore.init() with a try-catch block to prevent unhandled promise rejections which could lead to application crashes or inconsistent states. [important]

    relevant lineawait appStore.init();

    relevant fileresources/js/store/index.ts
    suggestion      

    Ensure that the removal of the promise-based initialization does not affect the execution order of dependent asynchronous operations, especially in environments with multiple asynchronous operations that depend on the completion of the initialization. [important]

    relevant lineif (this.config.network) {

    relevant fileresources/js/components/pages/auth/Login.vue
    suggestion      

    Verify that the direct use of await appStore.init() in the login flow does not introduce delays or affect user experience, especially in slow network conditions. Consider adding a loading indicator if not already present. [medium]

    relevant lineawait appStore.init();

    relevant fileresources/js/store/index.ts
    suggestion      

    After simplifying the init method, ensure that all error handling is robust, particularly for network requests within this.checkURL and ApiService.fetchInternalUrl, to prevent unhandled exceptions. [important]

    relevant lineconst urlConfig = await this.checkURL(this.config.url);

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a null check for urlConfig to prevent accessing properties of undefined

    Consider adding a check for urlConfig before accessing its properties to prevent potential
    runtime errors if urlConfig is undefined.

    resources/js/store/index.ts [76-77]

    -this.config.network = urlConfig?.network;
    +if (!urlConfig) return;
    +this.config.network = urlConfig.network;
     this.config.packages = Object.entries(urlConfig.packages).map(([key, value]: any[]) => {
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by adding a null check for urlConfig. This is a crucial improvement for preventing crashes if urlConfig is undefined.

    9
    Enhancement
    Encapsulate the async initialization in a try-catch block to handle errors

    Consider using try...catch around the asynchronous call to handle potential errors
    gracefully.

    resources/js/components/App.vue [42-44]

     (async () => {
    -    await appStore.init();
    -    initialTheme();
    +    try {
    +        await appStore.init();
    +        initialTheme();
    +    } catch (error) {
    +        console.error('Initialization failed:', error);
    +    }
     
    Suggestion importance[1-10]: 8

    Why: Adding a try-catch block around the asynchronous call is a good practice to handle potential errors gracefully, improving the robustness of the code.

    8
    Maintainability
    Refactor to reduce complexity and improve code readability

    Refactor the method to avoid deep nesting and improve readability by using early returns
    or separating concerns into smaller functions.

    resources/js/store/index.ts [66-74]

     const urlConfig = await this.checkURL(this.config.url);
    -try {
    -    const internalConfig = await ApiService.fetchInternalUrl(this.config.url);
    -    if (internalConfig) {
    -        this.internal = true;
    -    }
    -} catch {
    -    this.internal = false;
    -}
    +if (!urlConfig) return;
    +this.internal = await this.fetchInternalConfig(this.config.url);
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to refactor the method to avoid deep nesting and improve readability is beneficial for maintainability. However, it is not as critical as fixing potential runtime errors.

    7
    Performance
    Review the necessity and placement of await appStore.init(); to optimize flow

    Ensure that the await appStore.init(); is necessary before checking appStore.loggedIn
    since this might introduce delays or dependencies on the initialization process.

    resources/js/components/pages/auth/Login.vue [155-156]

    -await appStore.init();
     if (appStore.loggedIn) {
    +    await appStore.init();
     
    Suggestion importance[1-10]: 5

    Why: While the suggestion aims to optimize the flow, it may introduce logical errors if appStore.init() is necessary before checking appStore.loggedIn. This requires careful consideration.

    5

    @zlayine zlayine changed the title hotfix: remove init promise [PLA-1785] remove init promise May 31, 2024
    @enjinabner enjinabner merged commit 71bf9f7 into master May 31, 2024
    3 checks passed
    @enjinabner enjinabner deleted the hotifx/init-promise branch May 31, 2024 09:01
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants