flackr / scroll-timeline

A polyfill of ScrollTimeline.
Apache License 2.0
887 stars 82 forks source link

Add configurable initialization for origin matching #253

Open calinoracation opened 4 months ago

calinoracation commented 4 months ago

This adds origin matching as a configurable pattern. Right now it's draft as I think it also needs to have a vite config change as well to add a different build for the manual initialization route. I suppose though that could be a follow-up and this would solve for parsing all origins with * as discussed in https://github.com/flackr/scroll-timeline/issues/248#issuecomment-1947402872. It seems that we should have an option where it is the same behavior or configurable prior to this though if we want folks to be able to maintain 1:1 behavior with how it parses <link> tags today.

I was running into a Vite issue that's totally able to be worked around, but particularly you can't mix UMD or IIFE formats with other formats like ES, AMD or CJS. Maybe I'm totally overlooking the config, maybe we just do 2 independent builds. Will leave that to y'all!

Added on this PR:

import { initPolyfill } from '<source>/initialize.js';

initPolyfill('*'); // <-- default
initPolyfill('https://www.google.com'); // <-- match this exact origin
initPolyfill(); // <-- Only same origin, also applies with false or anything not truthy
initPolyfill(/airbnb\.com/); // <-- matches regexp
initPolyfill([<string> | RegExp]); // <-- matches based on above logic for string or regexp if any match
calinoracation commented 4 months ago

Was trying to think of an easier method, but with the IIFE perhaps we could return / export the function and if it is not executed in either a microtask or the next task we execute it. So something like:

import scrollTimelinePolyfill from '<cdn_or_package_location>';

if (userNeedsManualConfig) {
  scrollTimelinePolyfill('<same args>'); // <-- manual configuration
} else {
  // setTimeout(scrollTimelinePolyfill('*'), 0) is executed automatically if no call to the returned function was present
}

That would mean that parsing would occur slightly later, but it's super minimal, would reduce the amount of changes needed and still maintain pretty similar behavior as the current build.

flackr commented 4 months ago

This might work with a resolved promise which would I think would still allow configuration but still ensure that it is executed immediately before control is returned to the browser.

Running it in a setTimeout will likely have undesirable side effects, e.g.:

flackr commented 4 months ago

Another option would be to only defer the remote CSS fetching until a configuration call. Since this is inherently async anyways it would have no side effects.

calinoracation commented 4 months ago

Another option would be to only defer the remote CSS fetching until a configuration call. Since this is inherently async anyways it would have no side effects.

Think my only concern here is folks wouldn't get a good out of the box experience without some manual intervention on this path. Probably the ideal consumption is import and forget about the polyfill.

This might work with a resolved promise which would I think would still allow configuration but still ensure that it is executed immediately before control is returned to the browser.

This feels like a reasonable middle ground, good out of the box behavior for the majority of folks, and if folks really want to configure it deeply to reduce the amount of files parsed, they can do so roughly at the time of import and control it. I assume even calling configure twice (1st time automatic if they didn't, second manually at some point later in time) would cause any new requests to follow that behavior.

flackr commented 4 months ago

Another option would be to only defer the remote CSS fetching until a configuration call. Since this is inherently async anyways it would have no side effects.

Think my only concern here is folks wouldn't get a good out of the box experience without some manual intervention on this path. Probably the ideal consumption is import and forget about the polyfill.

To be clear, we would still also have the timeout / promise so you wouldn't have to do anything, but it would only defer the remote sheet fetches but everything else would still be parsed / processed allowing for e.g. wpt tests to still make synchronous assertions.

calinoracation commented 4 months ago

To be clear, we would still also have the timeout / promise so you wouldn't have to do anything, but it would only defer the remote sheet fetches but everything else would still be parsed / processed allowing for e.g. wpt tests to still make synchronous assertions.

Ah, that definitely makes sense now. I've updated the implementation to reflect that, and the fact we expect it is likely to be called multiple times so cleans up the observer when that happens. I haven't fully tested this yet (and no vitest for it quite yet), so not sure it's covered all the edge cases and it might be overly complex for what we need. Conceptually I think it matches what the direction is though.

calinoracation commented 4 months ago
Screenshot 2024-03-01 at 4 08 52 PM

Confirmed it's fetching for me now at the very least. I need to check a bit more though as some test failures and passes are different, and that seems unexpected.