Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More practical caching strategy for importHref #4200

Closed
wants to merge 4 commits into from

Conversation

cdata
Copy link
Contributor

@cdata cdata commented Dec 5, 2016

Reference Issue

Fixes #3908

Previously, importHref would cache failed results. This caused failures
to be cached indefinitely, even when the failure was the result of
conditions that are subject to change over time (such as poor network
connectivity).

This implementation no longer caches failures. Additionally, it relies
on the link tag's presence in the of the document as an
indication that it is loading or has loaded, rather than keeping an
object map of URLs to link tags.

document.head.appendChild(link);
} else if (link.__dynamicImportLoaded || !link.__dynamicImport) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale here is that imports that were loaded dynamically and finished, or weren't loaded dynamically to begin with, should fire a synthetic load event. This might fall apart for imports that were loaded outside of importHref, but aren't fully loaded yet by the time we get here. Thoughts?

imprt.addEventListener('error', errorListener);

if (optAsync) {
link.setAttribute('async', '');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional branch exists on its own to satisfy the constraints of a test. The test asserts that a link is imported async by checking for an async attribute on a link. If a link is imported synchronously at first, and then later imported asynchronously (as is the case in the test), then we must always set the async attribute even if we are relying on a "cached" (that is, already-done-loading) link tag for state.

Previously, importHref would cache failed results. This caused failures
to be cached indefinitely, even when the failure was the result of
conditions that are subject to change over time (such as poor network
connectivity).

This implementation no longer caches failures. Additionally, it relies
on the link tag's presence in the <head> of the document as an
indication that it is loading or has loaded, rather than keeping an
object map of URLs to link tags.
@gronke gronke mentioned this pull request Dec 9, 2016
sorvell pushed a commit that referenced this pull request Feb 10, 2017
* delay callbacks until HTMLImports.whenReady if it exists
* only avoid re-creating an import if there is an existing import created via importHref (detected via `import-href` attr)
@sorvell sorvell mentioned this pull request Feb 10, 2017
@gronke gronke mentioned this pull request Feb 28, 2017
@TimvdLippe
Copy link
Contributor

These changes have been incorporated in #4305, but this PR was not properly closed.

@TimvdLippe TimvdLippe closed this Aug 8, 2017
@TimvdLippe TimvdLippe deleted the refactor-import-href branch August 8, 2017 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants