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

Scripts begin loading immediately when you call thenRequire (#67) #74

Merged
merged 1 commit into from Feb 11, 2013

Conversation

wibblymat
Copy link
Collaborator

Doing this a pull request to get a review.

@addyosmani
Copy link
Owner

The implementation review lgtm so far, but it would be great to get a few more tests in :)

@wibblymat
Copy link
Collaborator Author

Tried to use Sinon to mock up the XHRs happening in the "wrong" order but I'm not sure if you can do that (I've asked about it at https://groups.google.com/forum/?fromgroups=#!topic/sinonjs/mcIGkiNZdcY).

Test case coverage still better now though.

@addyosmani
Copy link
Owner

Thanks for improving the test coverage of the implementation. I think that since we're not tight for time on the next release it would be useful to wait on a reply to the group thread. Alternatively, maybe there's a way a Sinon helper could be written that does provide the async. behavior you're after?

@addyosmani
Copy link
Owner

Let's chase this up again before we want to do another release. I think there's room for us to address both plugins and this for it.

@wibblymat
Copy link
Collaborator Author

I had a response to the Google Groups post I made but I had forgotten to turn on email updates so hadn't seen it.

I've updated the ordering test based on that. I don't have anything else I was intending to update on this so I'll squash the commits and merge when I get a moment.

(but only execute when the previous promise is fulfilled.)
wibblymat added a commit that referenced this pull request Feb 11, 2013
Scripts begin loading immediately when you call thenRequire (#67)
@wibblymat wibblymat merged commit 59df911 into addyosmani:gh-pages Feb 11, 2013
@wibblymat wibblymat deleted the issue-67 branch February 11, 2013 14:54
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.

None yet

2 participants