Add a conditional capability based loader capability #48
Conversation
There was a problem hiding this comment.
hello, i greatly appreciate your contribution and interest.
before we merge this, i'm going to ramble a little bit, so that hopefully we can share in some considerations. if you land on a solution that you are happy with, and that addresses the specific concerns in the other comments i've left, i'll be willing to merge your pull request.
oh, and one thing:
➡️ we should add a bit to the readme, explaining to users how to use this conditional installation pathway.
as you mentioned:
what about instead of immediately executing, we export a function like conditionallyLoadWebpHero
that might be a good strategy.
maybe, users of webp-hero could install a conditional-loading setup, based on the following example:
import {detectWebpSupport, loadScript} from "webp-hero/dist-cjs/conditionally.js"
const webpBundleUrl = "/node_modules/webp-hero/dist-cjs/webp-hero.bundle.js"
void async function main() {
const webpSupported = await detectWebpSupport()
if (webpSupported) {
await loadScript(webpBundleUrl)
const webpMachine = new webpHero.WebpMachine()
await webpMachine.polyfillDocument()
}
}()in the above case, conditionally.ts would merely need to re-export detectWebpSupport and a new loadScript.
the downsides of the above example:
- the url has to be hardcoded. hard to find a workaround for that.
- this doesn't include a plan for a simple html-only installation (only esm/commonjs).
- (like the rest of webp-hero's recommended installation pathways) it's a little clunky needing to include a block of javascript rather than just a simple script tag.
perhaps another way to think of this, would be to think of the ideal user experience, installing this via html-only
<script
defer
data-webp-hero
src="https://unpkg.com/[email protected]/dist-cjs/conditionally.bundle.js"
></script>i suppose the script could query for itself in the dom, find the src, do some regex-magic to find the appropriate url to the webp-hero.bundle.js.
i don't know. these are just some ideas.
lately, i'm not entirely satisfied with webp-hero, and have been thinking of a serious refactor to improve it's simplicity and ease-of-use (hopefully without any breaking changes) -- but i don't have the time to do so now, and probably not for awhile.
don't mean to be nit-picking -- if we can land on a solution that works for you, and that others like, we can certainly include it.
source/conditionally.ts
Outdated
| 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'; |
There was a problem hiding this comment.
i don't think we want to hardcode this, neither to the unpkg cdn, nor to 0.0.2 — we need a different strategy.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
source/conditionally.ts
Outdated
| isWebpSupported(function(support) { | ||
| if (! support) { | ||
| var js = document.createElement('script'); | ||
| js.src = 'https://unpkg.com/[email protected]/dist-cjs/polyfills.js'; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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 forArray - 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)
source/conditionally.ts
Outdated
| document.body.appendChild(js2); | ||
| js2.addEventListener('load', function() { | ||
| var webpMachine = new webpHero.WebpMachine(); | ||
| webpMachine.polyfillDocument(); |
There was a problem hiding this comment.
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 🤷♂️
There was a problem hiding this comment.
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.
|
❤️ Thanks for the feedback and thoughts @chase-moskal - I'll work through these soon. |
Sure, happy to take a pass at the readme update.
In the context of a CMS like WordPress the URL could actually be dynamic, eg. the script could easily be served from the origin domain.
I feel like having a way to set or pass the urls(s) is more flexible for developers who may want to serve the scripts from specific locations (for example building the detection into the site bundle, but loading the scripts from the CDN when needed).
Right, for the simplest use case, maybe it makes sense to have a single script to enqueue that automatically runs the check and loads and runs I'll review the code feedback next and work on iterating my patch. |
|
Thanks again for the code feedback and tips @chase-moskal - very helpful; I'll post an updated PR soon. Appreciate any help understanding this (probably basic) question - #48 (comment) |
|
thanks @adamsilverstein this will become the new official and recommended way to load webp-hero! please give me some time this week to play with it, and i'll get it merged soon 👍 i'll take it from here, thanks for your great contribution ❤️ |
|
hello @adamsilverstein if you have the time, can you please review my changes that i'm thining about making on your branch? please review this PR adamsilverstein#1 i guess it's weird to launch a PR onto another PR branch -- but, i also thought it's weird to push on somebody else's branch -- in retrospect i think i managed to pick the weirdest most-complicated option, and i think i regret it 🤷♂️ let me know what you think of the code! 🍻 |
Hi @chase-moskal I was working on testing capability based conditional loading for
webp-heroand found you were already considering adding something directly (#42 (comment)). So I decided to try and contribute that capability to your project (thanks for the amazing work by the way!)I worked up this PR as a proof of concept and have some questions:
conditionallyLoadWebpHerothat accepts the URLs for the bundle/polyfill and loads them if needed (using the cdn urls by default)?Related question: why are two scripts used, could this be built as a single script? Asking in part because dynamically adding two scripts is messy!