-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Fix 1530 - Asynchronously load scripts with JSX transformer #1558
Conversation
} | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: semicolon is unnecessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nixed, (amended to commit)
There aren't any tests for this, no. It would be cool to load the scripts in parallel but run them in order, but this is fine too. |
@@ -150,7 +150,7 @@ var load = exports.load = function(url, callback) { | |||
|
|||
// Disable async since we need to execute scripts in the order they are in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, can you update this comment too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// async, however scripts will be executed in the order they are in the
// DOM to mirror normal script loading.
sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Isn't this executing them in arbitrary order? |
@syranide |
@nhunzaker Thanks! |
Fix 1530 - Asynchronously load scripts with JSX transformer
@spicyj Glad to help! I have another branch out that loads scripts in parallel. I tried to keep things minimal, what are your thoughts on this?: https://github.com/nhunzaker/react/compare/jsxtransform-parallel?expand=1 |
@nhunzaker Looks pretty reasonable at a first glance. It might be nice to run scripts as soon as they're loaded if all the scripts before them have also loaded (but not if it'd add a lot of code complexity). Either way, could you open a PR with this when you're ready so I remember to look at it? Thanks! |
Is there a way to listen for JSXTransformer to finish processing all scripts? Previously I could add another onload listener and be guaranteed that it executed after JSX scripts were executed. That's no longer possible after this change. Any ideas? In my development environment, I have my UI logic in a jsx file and my app in a separate JS file which can only call |
You shouldn't really use JSXTransformer for anythimg serious at all. Use ./jsx --watch (super simple) or browserify/webpack instead. |
@fleaflicker Not currently, though it's possible we could add something for that if you open a separate issue, but if you load your second (pure-JS) file with JSXTransformer as well (using |
I was kind of curious about how this script worked, so I took a stab at #1530. The implementation is fairly simple, an enhancement would be to load all of the script tags in parallel and then
run
them sequentially.But, this gets the waterfall without synchronize requests on an example I had:
Also, I wasn't able to find tests for this script, do any exist?