kubetail-org / loadjs

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

Add support for more script attributes #28

Closed toddw closed 7 years ago

toddw commented 7 years ago

It's great that loadjs supports the async attribute but I need it to support crossorigin in an app that I am building. In looking at the MDN:script docks I noticed that some people might want support for the integrity tag as well.

I also found a nice way to test script tags for attributes by searching for them based on the script src with a unique query parameter added. Using this method might be easier than manually testing that async: false does what async: false is supposed to do but instead just trusting that if it was set the browser treats it as it should. Not saying that needs to change, just an idea as it is very easy to test for the attribute values using the method I used in the added tests.

amorey commented 7 years ago

@toddw Thanks for the PR and apologies for the late reply! I've been a bit preoccupied with the post-election conversation and your PR slipped through the cracks.

Adding support for crossorigin and integrity attributes is a really good idea and your modifications look great. However, one problem I see with the current solution is that the value of crossorigin and integrity applies to all the script tags and it's not possible to choose a different value per script. Do you have any thoughts on how we can address this issue?

amorey commented 7 years ago

@toddw Have you had a chance to think about my comments regarding per-script attributes? I had a couple of ideas I wanted to run by you:

  1. Attribute map

    loadjs(['/path/to/foo.js', '/path/to/bar.js'], {
    success: function() { /* foo.js and bar.js loaded in series */ },
    attrs: {
      '/path/to/foo.js': {
        crossorigin: true,
        integrity: 'sha256-Oibm40oTvUn9mOEKT7z9YccKewR2y4uHLTleMCUE2p0='
      },
      '/path/to/bar.js': {
        crossorigin: false,
        integrity: 'sha256-6CPSbvID6vU0eWdmSpaXiQghV16F4oCPADTjBevFtbI='
      }
    }
    });
  2. Before callback function

    loadjs(['/path/to/foo.js', '/path/to/bar.js'], {
    success: function() { /* foo.js and bar.js loaded in series */ },
    before: function(scriptEl) {
      if (scriptEl.src === '/path/to/foo.js') {
        scriptEl.crossorigin = true;
        scriptEl.integrity = 'sha256-Oibm40oTvUn9mOEKT7z9YccKewR2y4uHLTleMCUE2p0=';
      } else if (scriptEl.src === '/path/to/bar.js') {
        scriptEl.crossorigin = false;
        scriptEl.integrity = 'sha256-6CPSbvID6vU0eWdmSpaXiQghV16F4oCPADTjBevFtbI=';
      }
    }
    });

Let me know what you think!

toddw commented 7 years ago

Nice! Those look pretty good. Are either of those currently supported currently?

amorey commented 7 years ago

Neither are currently supported yet. Do you have any thoughts on which one would be better? The attrs map might be more straightforward but the before method could be really powerful (and a bit simpler to implement). I'm happy to implement it or I can leave it to you if you're interested in working on it. Let me know what you think!

toddw commented 7 years ago

I'm happy to submit a PR. More powerful and simpler to implement sounds pretty good. I'll try that first

amorey commented 7 years ago

Sounds great! I'll keep an eye out for the PR.