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

delay start() until init() happened. Fixes #358 #373

Closed
wants to merge 4 commits into from

Conversation

rodneyrehm
Copy link
Contributor

Prevent start() from running before init() had a chance to setup the config. This happened regularly in our RequireJS setup.

@@ -400,6 +400,15 @@ QUnit = {
},

start: function( count ) {
// QUnit hasn't been initialized yet.
// Note: RequireJS (et al) may delay onLoad
Copy link
Member

Choose a reason for hiding this comment

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

Mixed tabs and spaces.

// QUnit hasn't been initialized yet.
// Note: RequireJS (et al) may delay onLoad
if ( config.semaphore === undefined ) {
setTimeout(function() {
Copy link
Member

Choose a reason for hiding this comment

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

QUnit.init is called from QUnit.load which is called from addEvent( window, "load", QUnit.load );. Maybe a more efficient solution can be made based on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no way around checking if initialization has happened from within start() (unless init() switches out a void start() for the real deal). This is the case because start() may be called (and thus executed) long before init() got triggered by onload. To not mess with loading and initialization, I thought it best to simply delay start() to the point init() has happened.

This may not be the nicest possible solution, but it's the one not opening new cans of worms…

Not even using DOMContentLoaded for init() would've solved this, as that event is not available in older browsers and the current default behavior is to fallback to onload.

Copy link
Member

Choose a reason for hiding this comment

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

The check with config.semaphore is indeed not pretty, but I wasn't criticising that. I mean the setTimeout to try again every 13 milliseconds. Maybe it can use onload as well? Or one QUnit's own callbacks?

QUnit.load starts with running callbacks for event "begin". To ensure onload is finished, maybe try this:

if ( config.semaphore === undefined ) {
    QUnit.begin(function() {
        // This is triggered from top of QUnit.load, go async to let that function return first
        setTimeout(function() {
            start( count );
        });
    });
}

It is a bit more code, but should be a lot more precise and less "polling".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you still need setTimeout() because begin callbacks are executed before init() has completed. This still feels ugly. Maybe a new event init should be introduced?

As long as the implementation of runLoggingCallbacks() is not changed from iterating the actual list (instead of a copy or something), calling start() from within a begin handler works fine.

As for the config.semaphore check being ugly: we can introduce config.initialized = true; making this a bit more explicit.

Do you want me to make these changes?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how the implementation of runLoggingCallbacks is relevant. QUnit.init and QUnit.load themselves are not in the callback queue. Look at the source code.

Calling start directly from the QUnit.begin callback will not work as at that point neither init nor load has completed (config.semaphore will still be undefined), but the next tick after the QUnit.begin event, both QUnit.init and QUnit.load will be completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling start directly from the begin callback will not work as at that point neither init nor load has completed (config.semaphore will still be undefined), but the next tick after the begin event, both init and load are finished.

Registering a begin on begin actually starting works fine - http://jsbin.com/apodor/1/edit (might not be intended, but this currently works, at least in Firefox 17). And yes, it will run the test before init completed. Translating that to your proposal: http://jsbin.com/apodor/2/edit - everything looks fine.

So again, the question, should I make the changes?

Copy link
Member

Choose a reason for hiding this comment

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

That first example is not a representation of what you described earlier, you're calling QUnit.begin itself again inside of QUnit.begin. That is effectively the same as calling setTimeout within QUnit.begin, but without relying on undocumented behaviour.

Modified versions:

Yeah, go ahead :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe, I was trying to make the point that should someone call start within begin (whatever the reason) - it would still work as expected.

Anyways… do you want the hacky (begin, timeout) solution, or the new "init" event?

@Krinkle
Copy link
Member

Krinkle commented Dec 17, 2012

Let's go with the QUnit event handler for begin for now.

I'd like to avoid adding more events until we have a new event interface (putting it all in the root space makes it hard to maintain and confusing what are methods and what are event binders (e.g. does invoking QUnit.init initialise QUnit or bind an event handler?).

QUnit.begin(function() {
// This is triggered at the top of QUnit.load, push start() to the event loop, to allow QUnit.load to finish first
setTimeout(function() {
start( count );
Copy link
Member

Choose a reason for hiding this comment

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

jshint: [L409:C21] 'start' is not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I shouldn't have copy-pasted your code… :D

Copy link
Member

Choose a reason for hiding this comment

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

maybe I shouldn't have copy-pasted your code… 😉

@Krinkle Krinkle closed this in 7c57d2d Dec 17, 2012
@ghost ghost assigned Krinkle Dec 17, 2012
@Krinkle
Copy link
Member

Krinkle commented Dec 17, 2012

Thanks! Landed in 7c57d2d.

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

Successfully merging this pull request may close these issues.

2 participants