walmartlabs / little-loader

A lightweight, IE8+ JavaScript loader
MIT License
370 stars 28 forks source link

add cors support for modern browsers #49

Closed jknight12882 closed 8 years ago

jknight12882 commented 8 years ago

adds support for CORS (#48)

ryan-roemer commented 8 years ago

@jknight12882 -- I'm thinking we're going to want to go a different direction on this one rather than adding individual params for augmentation to the script tag (since we've had other requests.

@exogen -- What would you think about something like:

  /**
   * Load Script.
   *
   * @param {String}    src       URI of script
   * @param {Function}  callback  (Optional) Called on script load completion
   * @param {Object}    context   (Optional) Callback context (`this`)
   * @param {Function}  scriptCb  (Optional) Calls back with `script` object before adding to DOM.
   * @returns {void}
   */
  var _lload = function (src, callback, context, scriptCb) {
      // ... stuff ...

      // ... in both paths we end up with this line ...
      script.src = src;

      // Do a synchronous call here to allow mutation of new `script` object
      if (scriptCb) {
        scriptCb(script);
      }

      // ... stuff ...
  }

?

ryan-roemer commented 8 years ago

Alternate idea:

  1. make sure to setTimeout(/* ..*/, 0) the actual DOM appending code.
  2. return script synchronously from the call window._lload so caller can further mutate script before actual script call begins.

Thoughts @jknight12882 @exogen ?

jknight12882 commented 8 years ago

I think that's a much more flexible solution - wasn't sure how generic you wanted to make things

exogen commented 8 years ago

I was also tempted by the idea of returning script and delaying the append, but I don't like the alteration it would make to the sequence of events. If someone kicks off a _lload and then does a bunch of CPU-bound stuff, the browser would currently be able to do that script loading (I/O) in the meantime, but if we delay the DOM append it wouldn't. It's also imaginable that someone would currently depend on a new script element being in the DOM after running _lload.

I like introducing either a cors or scriptCb option but would prefer to pull the trigger on an options object now as it probably won't be the last time we need to introduce an option. IMO it should replace the second argument, callback, which we can detect the type of to still allow and translate the src, callback, context form.

ryan-roemer commented 8 years ago

@exogen --

So, something like this as a strawman?

  /**
   * Load Script.
   *
   * @param {String}            src               URI of script
   * @param {Function|Object}   [callback|opts]   (Optional) Callback or options object
   * @param {Object}            context           (Optional) Callback context (`this`)
   * @returns {void}
   */
  var _lload = function (src, callback, context) {
      var realCallback = typeof callback === "function" ? callback : callback.callback;
      var opts = (typeof callback !== "function" ? callback : {}) || {};

      // ... stuff ...

      // ... in both paths we end up with this line ...
      script.src = src;

      // Do a synchronous call here to allow mutation of new `script` object
      if (opts.scriptCb) {
        opts.scriptCb(script);
      }

      // ... stuff ...
  }

If so, let's hash out the opts object:

Option 1

{
  callback: function () {}
  scriptCb: function () {}
}

Option 2

{
  callback: function () {},
  // Just merge everything here....
  script: {
    crossOrigin: VALUE,
    id: VALUE
  }
}

Option 3

???

exogen commented 8 years ago

@ryan-roemer Yup, pretty much that logic with some cleanup! We can hash it out once code is PR'd – @jknight12882 do you have time & the desire to update to this approach?

I think the scriptCb approach makes the fewest assumptions and keep things nice & small for now. If more advanced handling is necessary in the future we could add a top-level option like crossOrigin that will potentially do other things beyond just setting a flag on script, but for now this is good enough (if a bit uglier for consumers to put in their code).

As for naming, I'd prefer the scriptCb option to be called setup.

ryan-roemer commented 8 years ago

OK, sounds good. So consensus is:

{
  callback: function () {}
  setup: function (/* script */) {},
  context: OBJ_OR_FALSY // bound to **both** `callback` and `setup`.
}

As @exogen mentioned, @jknight12882 you're welcome to code it up + a test or two (just maybe set an id element -- we don't need an actual elaborate CORS test).

If not, we'll get to it sometime soon-ish...

exogen commented 8 years ago

@ryan-roemer To add to that, I think context should be possible to pass in opts too; then fall back to the third arg if undefined in opts.

ryan-roemer commented 8 years ago

@exogen -- Sounds good. Updated my POC to include:

context: OBJ_OR_FALSY // bound to **both** `callback` and `setup`.

sound right?

jknight12882 commented 8 years ago

More than happy to tackle this. Just a thought though before I get started - I tend to be less excited about the current strawman proposal of having callback be either a function or an options object. One of the things that's great about little-loader is it's minimal file size and very clear and straight forward api. Having this function or opts object pattern seems like a lot of additional overhead for little benefit. I tend to think that the best approach is to have an optional 4th argument that is just the synchronous scriptCb (as originally proposed by @ryan-roemer). It introduces very little code churn into an already battle tested library and will have very minimal impact on the gzipped size.

ryan-roemer commented 8 years ago

OK, let's chat a bit more. My concern is:

  1. too many positional arguments
  2. not having all the args.

Use cases:

// Fire and forget - OK
_lload("http://foo.com/foo.js");

// callback - OK
_lload("http://foo.com/foo.js", callback);

// callback w/ context - OK
_lload("http://foo.com/foo.js", callback, this);

// callback w/ context + setup - OK (POSITIONAL)
_lload("http://foo.com/foo.js", callback, this, setup);
// callback w/ context + setup - OK (OPTS)
_lload("http://foo.com/foo.js", {callback, setup}, this);

// callback w/o context + setup - NOT OK (POSITIONAL)
_lload("http://foo.com/foo.js", callback, /* DUMMY VAR ? */, setup);
// callback w/o context + setup - OK (OPTS)
_lload("http://foo.com/foo.js", {callback, setup});

// callback w/ only setup - NOT OK (POSITIONAL)
_lload("http://foo.com/foo.js", /* DUMMY CALLBACK ? */, /* DUMMY VAR ? */, setup);
// callback w/ only setup - OK (OPTS)
_lload("http://foo.com/foo.js", {setup});

I think "opts" wins over "positional"

And the other concern: Are there any other use cases besides a script callback we'd want to add in the future? Because that then strongly says "do opts".

Thoughts all around?

jknight12882 commented 8 years ago

@ryan-roemer @exogen I've updated the pull request with the suggestions, lemme know what you think

jknight12882 commented 8 years ago

Closing this in favor of #50