-
Notifications
You must be signed in to change notification settings - Fork 100
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
base: master
Are you sure you want to change the base?
Add configurable initialization for origin matching #253
Conversation
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. |
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.:
|
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 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. |
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. |
5f62c88
to
598c10b
Compare
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: