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

Script loading parallelized in browser #3012

Merged
merged 4 commits into from Oct 20, 2013

Conversation

imcotton
Copy link
Contributor

turn this:
before
into this:
after

btw: they're actual screen capture, not PhotoShopping...

@vendethiel
Copy link
Collaborator

If this preserves execution order etc etc, I'm all for it :).

@imcotton
Copy link
Contributor Author

yeah, I know that I should have attach testing code against this feature, but quickly go though the test folder, it seems not easy to write formed unit test code for this async like behavior into existing structure, any suggestion would be great, or should I make one test page on jsFiddle maybe?

@vendethiel
Copy link
Collaborator

Yes, I'm afraid we don't have tests for this kind of stuff (and I'm not even sure how we should do it)

@imcotton
Copy link
Contributor Author

imcotton commented Jun 1, 2013

Hi, in order to make it easy to test, I've made a Cherry Pick from this PR onto HEAD @566a7da, which after v1.6.2 yet right before the module.exports got introduced from sourcemap.litcoffee, that breaks browser execution up to now, so please feel free to check it out at this branch, thanks.

@jashkenas
Copy link
Owner

Great idea -- but the implementation looks a bit over-complicated to me. Can we simplify a bit before merging?

@michaelficarra
Copy link
Collaborator

@imcotton: I'm not sure I understand your comment. Are you saying that 5496a18 broke something for you? Or are you implying that it wasn't sufficient to allow you to rebase off master?

@imcotton
Copy link
Contributor Author

imcotton commented Jun 2, 2013

@michaelficarra I mean before I start working on this, due to PR #2934 not yet been merged into master, so it couldn't get to run in the browser for testing purpose. And now I've tested with latest master branch, it is working now, so I don't know either your guys prefer rebase or merge to master on my side before this PR got picked up, thanks.

@imcotton
Copy link
Contributor Author

imcotton commented Jun 2, 2013

@jashkenas The code it now has get simplified, and I've performed rebase to current master HEAD @2e40864 for this PR, please let me know if there's still something left, thanks!

@imcotton
Copy link
Contributor Author

include rebuilt lib/coffee-script/browser.js, and rebased against latest master branch.

jashkenas added a commit that referenced this pull request Oct 20, 2013
Script loading parallelized in browser
@jashkenas jashkenas merged commit db87d81 into jashkenas:master Oct 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants