search

kubetail-org / loadjs

A tiny async loader / dependency manager for modern browsers (899 bytes)
MIT License
2.58k stars 150 forks source link

Support for `before` callback for before scripts are embedded #29

Closed toddw closed 7 years ago

toddw commented 7 years ago

Execute a callback before script tags is embedded

  loadjs(['/path/to/foo.js', '/path/to/bar.js'], {
    before: function(path, scriptEl) { /* called for each script node before embedded */ }
  });

Reference

https://github.com/muicss/loadjs/pull/28#issuecomment-266206138

amorey commented 7 years ago

💯 Looks great!!!

I didn't realize that Chrome returns the full url as src. Would it help to pass the original path to callback argument (e.g. beforeCallbackFn(path, scriptEl))?

toddw commented 7 years ago

Great idea! I updated the beforeCallbackFn to get the path as the first argument as you suggested. Let me know if there is anything else.

Question: For future reference, do you prefer we run ./node_modules/.bin/gulp dist:build for PRs or would you prefer it be just the code change to src/loadjs.js?

toddw commented 7 years ago

I also updated my README example, what I had previously was incorrect.

amorey commented 7 years ago

Looks great! I merged your PR into the master and bumped the version number (v3.2.0).

In terms of PRs, it makes things a bit easier if you submit changes without running dist:build first but the loadjs code is simple enough that it's not a huge deal. I usually run test:build in branches and then run build-all/dist:build only before publishing a new version. I think this is an issue with a lot of PRs so let me know if you've seen any better ways of handling it.

Thanks again for bringing up the issue of additional attributes and for your PRs! I really appreciate the time you put into the code and the unit tests. If you're looking for other javascript-heavy projects to work on please take a look at my other project, MUI, which is a lightweight CSS framework (https://github.com/muicss/mui). It's been getting some attention and I can definitely use some help on it!

toddw commented 7 years ago

When you're publishing to NPM you can use a prepublish script to create the build but I have a feeling your users are installing from a variety of ways so that probably wouldn't be a great option for you as far as running the build script.

Anyways, thanks for merging and the feedback on approach! This was fun, I'll definitely check out MUI