Add edge-case tests for script and style loading

[nicksay]

In review for #46, @DavidCPhillips raised the issue that the current unit tests for script and styling loading only cover the expected cases:

Regarding resource_test.js:

These pretty much only hit the happy path cases. It'd be nice to see some tests for name mismatches (existing url and new name or vice versa) and multiple urls that are already partially loaded.

Regarding script_test.js:

On the other hand, these are now pretty trivial functions. I, for one, wouldn't mind removing their tests altogether.

[nicksay]

I started looking at this a bit today and realized that we do not explicitly handle the "existing url and new name" case. That means:

1. load with an existing url and new name will not add the js/css
   • a spfjsunload or spfcssunload event will not be dispatched
2. unload with either the new name or old name will remove the js/css
   • a spfjsunload or spfcssunload event will be dispatched for the name given

I'm thinking that 1 is okay, considering the resource is the same. But it'd be good to handle 2 better.

We have a couple options:

1. Only allow a single name for any given resource, replacing the old one
2. Change unloading to dispatch events for all names registered before removing the resource
3. Change unloading to not do anything until all names are unloaded
4. Change unloading to dispatch events but not remove the resource until all names are unloaded

When thinking about this, we should consider CSS loading/unloading first, since the issue of when to actually remove the resource matters. In the options below, I've included the event dispatches, to help illustrated the differences between the names changing and the actual resources be added/removed. In practice, the "unload" are much more likely to happen as part of a navigate.

Option 1 looks like:

spf.style.load(url, 'foo');
// register name 'foo'
// add style
spf.style.load(url, 'bar');
// unregister name 'foo'
// register name 'bar'
spf.style.unload(url, 'foo');
// no-op
spf.style.unload(url, 'bar');
// remove style
// dispatch 1 event:
//     spfcssunload 'bar'
// unregister name 'bar'

Option 2 looks like:

spf.style.load(url, 'foo');
// register name 'foo'
// add style
spf.style.load(url, 'bar');
// register name 'bar'
spf.style.unload(url, 'foo');
// remove style
// dispatch 2 events:
//     spfcssunload 'foo'
//     spfcssunload 'bar'
// unregister name 'foo'
// unregister name 'bar'
spf.style.unload(url, 'bar');
// no-op

Option 3 looks like:

spf.style.load(url, 'foo');
// register name 'foo'
// add style
spf.style.load(url, 'bar');
// register name 'bar'
spf.style.unload(url, 'foo');
// unregister name 'foo'
spf.style.unload(url, 'bar');
// remove style
// dispatch 1 event:
//     spfcssunload 'bar'
// unregister name 'bar'

Option 4 looks like:

spf.style.load(url, 'foo');
// register name 'foo'
// add style
spf.style.load(url, 'bar');
// register name 'bar'
spf.style.unload(url, 'foo');
// dispatch 1 event:
//     spfcssunload 'foo'
// unregister name 'foo'
spf.style.unload(url, 'bar');
// remove style
// dipsatch 1 event:
//     spfcssunload 'bar'
// unregister name 'bar'

[nicksay]

@DavidCPhillips Thoughts?

[rviscomi]

My two cents: I don't like Option 4 because it dispatches an unload event without actually unloading the style. I like Option 3 because unload considers each preceding load invocation before finally removing the style.

[DavidCPhillips]

Like @rviscomi, I also don't like option 4 for the same reason. I also don't really like Option 3 either though, since it can lead to unexpected scenarios for the users.

Not receiving the unload event isn't a very strong signal to a user that their resource wasn't actually unloaded. So, if a resource was loaded twice with different names, one part of the code could ask for it to be unloaded and assume its gone. For a stylesheet, this would be especially bad behavior when unexpected clashes start rising.

I'd rather go with Option 1, with a nice strong log warning when this occurs. I think Option 2 is reasonable as well, but it might be harder for end users to detect problems and fix them.

[nicksay]

Option 1 is now out for review in https://github.com/youtube/spfjs/pull/153