Skip to content

Conversation

@danielbreves
Copy link
Contributor

@zendesk/vegemite

Increase promise timeout to 10 seconds to allow more time for requests to resolve. It seems especially during initialisation 5 seconds may not be enough for requests to complete when many apps are loading at the same time.

References

ZD#4058685

Risks

  • [low] In case there's a legitimate reason for postMessage to never resolve it will take longer for the app to be able to handle the error.

@danielbreves danielbreves force-pushed the daniel/increase-promise-timeout branch from 6fad941 to f24590a Compare November 20, 2018 01:09
Copy link
Contributor

@hoangtrvu hoangtrvu left a comment

Choose a reason for hiding this comment

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

Other than comment 👍

lib/client.js Outdated
var PROMISE_TIMEOUT = 5000
// 5 seconds
var PROMISE_TIMEOUT = 10000
// 10 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reference the ticket here so we have some context if next time we consider changing this again.

Copy link
Contributor

@Mehul-Porwal Mehul-Porwal left a comment

Choose a reason for hiding this comment

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

👍 on 🍏

@danielbreves danielbreves force-pushed the daniel/increase-promise-timeout branch from da5afde to 81fc76b Compare November 20, 2018 01:54
@danielbreves danielbreves merged commit 4d9fc2a into master Nov 20, 2018
@danielbreves danielbreves deleted the daniel/increase-promise-timeout branch November 20, 2018 02:09
@bryan-flynn-zd
Copy link

Hi @danielbreves @Mehul-Porwal -- when will this be rolled out? Coherence via ticket https://support.zendesk.com/agent/tickets/4058685 really wants this as soon as they can get it.

I'm assuming, too, that existing zaf_sdk assets will be updated in place, correct? (IOW, Coherence won't have to reference a different SDK version or location)

@hoangtrvu
Copy link
Contributor

Hi @bryan-flynn-zd, yes that's correct, they won't have to update their reference to the SDK. We're trying to see if we could deploy this today.

@zendesk-deploy
Copy link

This PR was deployed to Static Assets Build. Reference: v2.0.22

@zendesk-deploy
Copy link

This PR was deployed to Static Assets Switch Staging. Reference: v2.0.22

@zendesk-deploy
Copy link

This PR was deployed to Static Assets Switch Major (v2). Reference: v2.0.22

@bryan-flynn-zd
Copy link

Hi @hoangtrvu -- I see the deploy but when I look at the actual asset files, I still see the 5000ms timeout value in both the older and newer locations:
https://static.zdassets.com/zendesk_app_framework_sdk/2.0/zaf_sdk.js
https://static.zdassets.com/zendesk_app_framework_sdk/2.0/zaf_sdk.min.js

Am I missing something?

@Mehul-Porwal
Copy link
Contributor

@bryan-flynn-zd I think it is just a browser cache. Opening it in incognito gives this:

screen shot 2018-12-04 at 10 32 31 am

@bryan-flynn-zd
Copy link

I see it now -- thanks @Mehul-Porwal !

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.

6 participants