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

Add configurable initialization for origin matching #253

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

Conversation

calinoracation
Copy link
Contributor

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 #248 (comment). 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
Copy link
Contributor Author

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
Copy link
Owner

flackr commented Feb 27, 2024

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.:

  • Many wpt tests expect to use it immediately and could fail
  • It's possible that you could miss the animationstart event of animations, resulting in some animations not being upgraded to their scroll driven timeline.

@flackr
Copy link
Owner

flackr commented Feb 27, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

flackr commented Feb 28, 2024

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 calinoracation marked this pull request as ready for review March 1, 2024 18:46
@calinoracation
Copy link
Contributor Author

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
Copy link
Contributor Author

calinoracation commented Mar 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants