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.

Sign up for GitHub

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

Add a conditional capability based loader capability #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add a conditional capability based loader capability #48

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a61f761
Add a conditional auto-loader
adamsilverstein May 18, 2022
b8e3210
Add loadScript; add load-webp-hero-conditionally.js to build
adamsilverstein May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions source/conditionally.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
declare const webpHero;

/**
* Check if WebP is supported by the current browser.
*/
export function isWebpSupported(callback) {
chase-moskal marked this conversation as resolved.
Show resolved Hide resolved
var img = new Image();
img.onload = function () {
var result = (img.width > 0) && (img.height > 0);
callback(result);
};
img.onerror = function () {
callback(false) ;
};
img.src = "data:image/webp;base64,UklGRiIAAABXRUJQVlA4IBYAAAAwAQCdASoBAAEADsD+JaQAA3AAAAAA"; // Lossy WebP image.
}

(function() {
isWebpSupported(function(support) {
if (! support) {
var js = document.createElement('script');
js.src = 'https://unpkg.com/[email protected]/dist-cjs/polyfills.js';
Copy link
Owner

Choose a reason for hiding this comment

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

i think we should leave polyfills outside the specific scope of the conditionally script -- because an application author will want to avoid double-loading polyfills, and they may want to take responsibility to load their own polyfills for the rest of their application (and not use webp-hero's provided polyfills at all)

Copy link
Author

Choose a reason for hiding this comment

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

i still don't understand what the polyfills do exactly vs the bundle :(, is that documented somewhere? So are you saying we can leave off the polyfill loading here entirely?

Copy link
Owner

Choose a reason for hiding this comment

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

@adamsilverstein sorry about the late reply,

yes, we should probably not engage in loading any polyfills via the conditionally script.

  • the polyfills include one for Promise, and another for Array
  • the inclusion of polyfills should be deferred to the author of the html page, who may decide to include their own polyfills for the other parts of their application, so we want to leave it in their hands (to avoid double-loading any polyfills)

document.body.appendChild(js)
js.addEventListener('load', function() {
var js2 = document.createElement('script');
js2.src = 'https://unpkg.com/[email protected]/dist-cjs/webp-hero.bundle.js';
Copy link
Owner

Choose a reason for hiding this comment

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

i don't think we want to hardcode this, neither to the unpkg cdn, nor to 0.0.2 — we need a different strategy.

Copy link
Author

Choose a reason for hiding this comment

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

I wonder could the default url just be relative the the current document? eg src='./webp-hero-bundle.js'? Could be overridable by setting a variable before loading.

Copy link
Author

Choose a reason for hiding this comment

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

hmm, that probably won't work actually because it would be relative to the HTML document. Maybe back to your regex idea, or even a simple split off of the last "/". I'll try that.

document.body.appendChild(js2);
js2.addEventListener('load', function() {
var webpMachine = new webpHero.WebpMachine();
webpMachine.polyfillDocument();
Copy link
Owner

Choose a reason for hiding this comment

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

the rest of the webp-hero installation techniques insist on the user having the flexibility to control the webpMachine as they see fit -- whereas this conditionally makes the hardcoded assumption that the user wants to call polyfillDocument -- surely most do, but some may want to use conditionally while also using the webpMachine differently 🤷‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

my goal here was to create a simple way for site to load a single script (ideally from a CDN) that would test for the capability, then load webp-hero and polyfill the document if needed. essentially all sites would need to do is include this single script to get performant webp support.

I will rework this with the async "loadScript" helper you suggested (in a separate file) - that way developers could build their own controlled load like your code sample, or they could include this single conditionally file from a cdn to get automatic compatibility.

});
});
}
});
})();
1 change: 1 addition & 0 deletions source/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* @file Automatically generated by barrelsby.
*/

export * from "./conditionally";
chase-moskal marked this conversation as resolved.
Show resolved Hide resolved
export * from "./convert-binary-data";
export * from "./detect-canvas-reading-support";
export * from "./detect-webp-support";
Expand Down