proposal for better support for custom swap functions

[martrapp]

Changes

Within the astro:before-swap event, people can replace the implementation of the swap function with their own version. By calling the original swap(), it is rather simple to add own code in front or after it.

But if you want to change one part of the implementation (e.g. how the body is replaced), you have to rewrite the whole swap() (handling of scripts, root attributes, and header elements) just to bring in your change for the handling of body elements.

This PR proposes to make the swap() blocks accessible for custom implementations.
The defaultSwap() implementation calls the blocks directly, without overhead compared to the previous handling.

Edited: See test on how to use the reusable building blocks.

Docs

No official API yet.

[changeset-bot]

⚠️ No Changeset found

Latest commit: e78fd3b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Fryuni]

Very interesting idea! I think that requiring the user to write over the function is not ideal though. I think something along this line would be nice:

class BeforeSwapEvent {
  // ...
  public swap: (swapper?: CustomSwapper) => void;

  public setSwapper(swapper: CustomSwapper) {};
}

I think this can be made backwards compatible while providing a cleaner API:

	document.addEventListener('astro:before-swap', (e) => {
		e.setSwapper(new CustomSwapper({
			swapHeadElements(doc) {
				const dynamicStyle = document.head.querySelector('style:not(:empty)');
				swapHeadElements(doc); // reuse the built-in implementation
				dynamicStyle && document.head.insertAdjacentElement('afterbegin', dynamicStyle);
			},
		}));
	});

[martrapp]

Hi Luiz!
@Fryuni: I very much welcome and appreciate the wizard's feedback!

IIUC, you say there should be a better way than

e.swap = () => new CustomSwapper({...}).swap(e.newDocument)

Chance are good, that this is mainly a naming issue. The class does not really swap anything but generates a swap function, thus overriding e.swap feels natural to me (and what we already have if you want to change the implementation of the built-in swap)

How about:

e.swap = new CustomSwapGenerator({...}).generate(e.newDocument)

Looking at your proposal, what I do not yet understand: Where does the custom swap function get doc bound to event.newDocument?
Would calling setSwapper( new CustomSwapp({...})) do something like
e.swap = swapper.swap.bind(swapper, event.newDocument)?

[Fryuni]

My worry is more about the composability. If you want to add a behavior on top of the existing swap that can be composed with other components, it would break with this API, or you have to go back to the dance of function value swaps.

For example, if I want to make a component that requires custom logic to persist, I need to do this today:

	document.addEventListener('astro:before-swap', (e) => {
        const nextSwap = e.swap;
		e.swap = () => {
          // custom state persistence
          nextSwap();
        };
	});

This way, I can add my logic without removing the logic from other components that do the same thing. You either clearly override everything that came before or you add something before/after it.

With the e.swap = () => new CustomSwapper({...}).swap(e.newDocument), it replaces the swap with a customization over the default swap logic, not the swap logic that was in the event. So, although it gives a nicer API for a component to write its own customizations, it makes those components not play well with other components on the page since it overrides everything.

I'd like a new API to make things easier without requiring those using it to override everything (being a good neighbor in the community has been my motto recently). That was the idea, not the naming.

I sketched another idea (here) that I think is even simpler and solves a few problems:

• Using the new API does not force you to override previous changes
• It doesn't need any for the saveFocus (long live type-safety)
• Allows modifications to be incremental, so you'd be able to plug and play components on the head that override each of the steps in the custom swapper.

And it follows the same design we have for middlewares, each one receiving the function to call the next.

[martrapp]

Oh dear, it really took me a while to realize what you had in mind! 🤦🏻‍♂️

To be honest, I didn't even think about composability when I came up with the solution for replacing parts of the built-in implementation, as I assumed that this would only be needed in special cases anyway. It was not meant as a replacement for decorating e.swap(). I primarily wanted to free authors from having to copy & own all the code of the built-in implementation and having to update it whenever the original changes (or even worse, diverge) in case where decorating could not help and you really have to change the the base implementation (as for example for a swap that only replaces the <main> section of a page)

Of course, now that I see how elegant your solution is, I regret that we didn't come up with something like this in the original swap.

[martrapp]

Many thanks @Fryuni! A real improvement indeed. It's always a great pleasure to develop things together with you ;-)

[martrapp]

It would also be easy to to deprecate e.swap and add swap to the SwapperExtensions.
But I guess that would be too confusing.

So the current story is:

• if you want to decorate the existing swap with some before and/or after code, redefine e.swap and call the origimaö version
• if you want to replace e.swap with logic independent of the built-in swap, do so. Others can still decorate it.
• if you need some change in the inner workings of the built-in swap, use e.swapper.extend({...})

Extensions are applied first, redefinitions of e.swap are applied second, see e2e chaining test.

Could need some help with good wording. How about:

rename CustomSwapper to BuiltInSwapExtender
rename e.swapper to e.builtInSwapExtender

[Fryuni]

It would also be easy to to deprecate e.swap and add swap to the SwapperExtensions. But I guess that would be too confusing.

I'm totally on board with deprecating e.swap, but since we can't actually remove it until Astro 5, then I don't think it will bring much value.

So the current story is:

• if you want to decorate the existing swap with some before and/or after code, redefine e.swap and call the origimaö version
• if you want to replace e.swap with logic independent of the built-in swap, do so. Others can still decorate it.
• if you need some change in the inner workings of the built-in swap, use e.swapper.extend({...})

Yeah, sounds great!

Extensions are applied first, redefinitions of e.swap are applied second, see e2e chaining test.

Maybe also worth pointing out that decorations and redefinitions are applied in the order of the listeners, so:

• A decorates
• B redefines
• C decorates
• D decorates

What will run is D decoration -> C decoration -> B redefinition. A and the default logic won't run.

Could need some help with good wording. How about:

rename CustomSwapper to BuiltInSwapExtender rename e.swapper to e.builtInSwapExtender

Some ideas without much thought:

• e.phases/e.phases.extend/TransitionSwapPhases
• e.hooks/e.hooks.extend/TransitionSwapHooks
• e.steps/e.steps.extend/TransitionSwapSteps

[martrapp]

e.steps/e.steps.extend/TransitionSwapSteps

That's the one! I'll take that! Merci!

[bluwy]

This API seems nice to me 👍 I think patching .swap is fine for me, but if it's common for users to modify how swapping works, then I think it's definitely worth making it nicer, like the .extends.

[ematipico]

I'll have to seat this one out, as I don't have much context. However, it's not very clear what's the plan of this PR. Is it something new? A patch/minor?

[matthewp]

@ematipico this would be a minor

[matthewp]

The .extend method is a bit odd to me. Before ES6 it was common to create classes using a .extend method (for example: https://api.jquery.com/jQuery.extend/). It's also not very webby, imo. Events don't have methods like this.

Here's a suggestion:

1. Leave e.swap as it previous was.
2. Add a new method e.swapWith (inspired by respondWith) that takes the swap hooks as an object like:

e.swapWith({
  swapHeadElements(doc) { ... }
})

If you call this method you don't need to replace e.swap. We can remember that the swap already occurred. Also we can get rid of the CustomSwapper, etc. classes as an API and just pass the hooks directly. Maybe it still exists as internal impl but I don't see a reason to expose it.

What do you think?

[Fryuni]

It breaks composability with one listener undoing what other is doing.

Same problem as I pointed out in this comment (#10908 (comment)) with the added problem of the swap being applied more than once if more than one listener calls swapWith.

[martrapp]

Let's take one step back and look at the original requirement.

From the five events we have for view transitions, astro:before-swap isn't the most prominent. Most use cases of this event might not involve redefining swap. Those that do, typically add some before or after code to swap and keep the default implementation. Only a small fraction want to override the default implementation. And even a smaller fraction need to override with a version that is 85% identical to the built-in implementation. This rare use case is what we are talking about. The trigger was an issue where the alternative was the definition of an own package forked off from astro.

The solution does not require a high-end API, but a simple, maybe even a bit hidden, way to benefit from the built-in implementation. Imho we should just keep the current event and the possibilities to override swap as is. In addition we offer a way to generate a swap function by extending the built-in implementation. This way we can still have all the benefits of Fryuni's improved proposal.

[martrapp]

Please note: From what I wanted to accomplish with this PR, I would also be perfectly fine with deleting swap-steps.ts and just keeping swap-functions.ts as a semi-official interface to reuse the building blocks of the built-in swap.

[matthewp]

Yeah great points all around @Fryuni, @martrapp. If multiple listeners are all doing swaps I think someone is going to have a bad time. Rather than dealing with the matrix of possibilities where multiple listeners are swapping a subset of parts and us trying to manage all of that, I do like the idea that you're proposing @martrapp of just exposing the raw functions and not being in the middle of making sure parts are only swapped once (removing the swap-steps in that case). I think this is a good starting point.

[martrapp]

I think this is a good starting point.

Done! Thank you all very much for the fruitful discussions!