Fix: Promises reject on failure

[stramel]

Addresses #20

Changes:

• If there are no imports found with the specified group, it will reject with empty arrays for loaded and failed.
• If there are any failed requests, it will reject with the results of the group imports.

@garlicnation @justinfagnani @rictic PTAL

UPDATE: Rebased onto master

[stramel]

Friendly Ping @justinfagnani

[web-padawan]

shouldn't this be placed inside of the following IIFE?

[stramel]

Yes, it should. I have been writing in ES6 and haven't actually used 'use strict' for a while. Good catch, thanks!

[justinfagnani]

I'm a little unsure of this behavior around failures, and whether it's coherent with regular HTML imports.

The normal use of HTML imports is not to listen for load and error events, but to have a scripts tag following imports that runs when the imports are complete, whether they failed or not:

<link rel="import" href="foo.html">
<link rel="import" href="bar.html">
<script>
  // this runs regardless of the success of importing foo.html or bar.html
  // but runs after both either loaded or failed.
</script>

I see two arguments in favor of handling errors differently:

1. It might be good to match the default behavior of HTML imports:

<dom-module>
  <link rel="lazy-import" href="foo.html" group="lazy">
  <link rel="lazy-import" href="bar.html" group="lazy">
</dom-module>

this.importLazyGroup('lazy').then(() => {
  // this runs regardless of the success of importing foo.html or bar.html
  // but runs after both either loaded or failed.
});

2. Lazy imports are more likely to fail, because the user could have gone offline since the app loaded.

Let me think a bit, and let me know if you have any thoughts on those arguments.

[justinfagnani]

@stramel the option I was thinking of was to resolve with an array of results - the load or error events. Though we have to think about what that means when we don't even call importHref because the import is already loaded.

[stramel]

@justinfagnani I believe that is how it functions currently. Resolves regardless with a result of imports. If the imports have already been loaded it still returns them as "loaded".

The current approach requires "boilerplate" for most implementations that want to gracefully fail the UI. You can see https://github.com/PolymerElements/polymer-starter-kit/pull/998/files#diff-33ee33aeb8ac3d69c66204db965c3330R165

this.importLazyGroup(page).then((results) => {
  if (results.failed.length > 0 || results.loaded.length < 1)
    this._showPage404();
});

vs

this.importLazyGroup(page).catch(() => {
  this._showPage404();
});

[justinfagnani]

I think that snippet above is likely incorrect. There usually shouldn't ever be a 404 of a lazy import - most of the time it's a URL pointing to a static resource. More likely the client is offline.

It's possible that a lazy-import could point to a dynamic, possibly unknown resource, but the linter would complain about it, and that's a case where importHref makes a lot more sense.

This is what I mean about Promise behavior not corresponding to the static eager import behavior:

<link rel="import" href="foo.html">
<link rel="import" href="bar.html">
<script>
  // this runs regardless of the success of importing foo.html or bar.html
  // but runs after both either loaded or failed.
</script>

[stramel]

@justinfagnani I agree there shouldn't be with the exception of offline and incorrect group name.

The linter wouldn't catch the case as I proposed in the PSK where page is the group being passed in from the function. (Navigating to /abc would try to fetch abc group and fail loading)

Also, I'm not sure there is a linting rule for lazy-imports to check static lazy group imports.

WRT importHref, how would you promisify that function? I would assume that you would do something similar to:

const resolvedPageUrl = this.resolveUrl('my-' + page + '.html');
new Promise((resolve, reject) => {
  Polymer.importHref(
    resolvedPageUrl,
    resolve,
    reject,
    true);
}).then(() => {
  console.log('success');
}, () => {
  console.error('fail');
});

To me, it kinda feels more like that...

I do get what you are saying about matching closer to the spec and I would love to match the spec. It just seems like more of a burden to have to check to see if something failed in the results while in the "success" function of the promise.

[elvomka]

lazy-imports has a declarative part, but in order to actually trigger the loading process, we need to use imperative code and invoke importLazyGroup(). IMO it would make sense to keep the error-semantics very close to importHref.

If lazy-imports only allowed a single HTML import at a time, semantics would be straight forward and the promise should be rejected if the import could not be performed.

The grouping feature makes matters a bit more complicated, but I think one could argue that a failing import would mean some sort of error-condition, which should be reflected explicitly in a rejection of the promise.

[stramel]

@justinfagnani Ping

[justinfagnani]

Ok, I'm convinced :)

[justinfagnani]

Regarding the specific implementation, I think we should discuss my latest comment in #29 about not explicitly keeping loaded state, but just storing the pending Promises.

[stramel]

I agree. Will wait until #36 lands before rebasing and adjusting this PR to work with it.

[stramel]

@justinfagnani Updated based on the merge of #36 with test updates and added a test to check the edge case of #36 as well.

[justinfagnani]

Reject should be called with an error though. The error could carry the statuses I guess.

[justinfagnani]

Can you rebase?

[stramel]

@justinfagnani Did you get a chance to circle back to this?

[stramel]

Ping @justinfagnani

[justinfagnani]

LGTM! Thanks!