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

Adapt.wait should implement a timeout in order to prevent blocking of rendering #2439

Closed
brian-learningpool opened this issue May 20, 2019 · 2 comments
Assignees

Comments

@brian-learningpool
Copy link
Member

Subject of the enhancement

The Adapt.wait API has begin() and end() calls but in cases where calls to begin() do not have a corresponding call to end() (for whatever reason) it can result in a course failing to load and difficult to diagnose errors.

Your environment

Adapt Framework v3.5.3

Steps to reproduce

Inside a plugin call Adapt.wait.begin() and do not call Adapt.wait.end().

Expected behaviour

After a given timeout, the queue should be flushed together with a debug message logged to the console to indicate as such.

Actual behaviour

The Adapt content appears to get stuck in an intermediate state between loading and rendering.

@brian-learningpool brian-learningpool changed the title Should Adapt.wait should have a timeout? Should Adapt.wait have a timeout? May 20, 2019
@brian-learningpool brian-learningpool self-assigned this May 21, 2019
@brian-learningpool brian-learningpool changed the title Should Adapt.wait have a timeout? Adapt.wait should implement a timeout in order to prevent blocking of rendering May 21, 2019
@brian-learningpool
Copy link
Member Author

Just discussed this with @oliverfoster on Gitter chat. Adapt.wait.for(callback) (and correctly handling the error callback) would have prevented this, but it does make sense to add a timeout.

A 7-second timeout (as used by RequireJS) would be sufficient. This should be reset on every call to begin(). Also, calls to this.end should be wrapped in a _.once().

brian-learningpool added a commit that referenced this issue May 24, 2019
To prevent an unhandled expection or a call to begin() not having a corresponding end() from blocking rendering of the Adapt content, a 7-second timeout is now added on every begin.  If the time has elapsed since the last begin() the outstanding calls to Adapt.wait.end() will be triggered.

Also added a _.once() call to the for() callback and replaced double quote marks in strings with single quotes.
@moloko moloko closed this as completed in 0cc00b9 Jun 11, 2019
@oliverfoster
Copy link
Member

@brian-learningpool are we ok to remove the 7 seconds now that the xpi begin/wait queue is sorted? or was this for something else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants