Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Add template.html transform api #1642

Conversation

happycollision
Copy link
Contributor

@happycollision happycollision commented Nov 22, 2020

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

This api addition to @sapper/server allows an escape hatch to arbitrarily transform the contents of the template.html file as it is being served. In fact, it moves the tag replacements that Sapper already does into this new system.

This was done largely to resolve #179, but also resolves #1036: allow access to arbitrarily change the template file as needed. I realize (somewhat after the fact) that there are already two other PRs open that achieve a similar thing (#1037, #1152), but this change allows arbitrary changes to the template (like #1152) and is not tied to adding a new middleware before the Sapper middleware runs.

I know that SvelteKit is on the way, but this change was small enough that I am hoping something similar might be possible there.

Here is the documentation I added so you'll better understand what I have done.


Sapper provides a few tags that it will automatically replace inside the src/.template.html file. These replacements can be changed by providing your own template transformer.

You can register a template transformer inside the src/server.js file.

	import polka from 'polka';
	import * as sapper from '@sapper/server';
+	import { makeHash } from './my-helpers';

	import { start } from '../../common.js';

+	sapper.registerTemplateTransformer((template) => 
+		template.replace(
+			'%mytag.globalCss%',
+			() => 'global.css?v=' + makeHash('global.css')
+		)
+	)
+
	const app = polka()
		.use(sapper.middleware());

	start(app);

The function that you pass to registerTemplateTransformer will be given two arguments:

  1. template: string contents of the template before Sapper's replacements
  2. data: an object containing all the values you'd need to completely replace the tags that Sapper itself replaces:
    • html: string
    • head: string
    • styles: string
    • script: string
    • nonce_value: string
    • nonce_attr: string
    • req: the request object

You must return a string which is the full contents of the template file as you'd like them to appear. Any of Sapper's own tags that you do not replace will be replaced as normal by Sapper.

type Transformer = (
	template: string,
	data: {
		html: string
		head: string
		styles: string
		script: string
		nonce_value: string
		nonce_attr: string
		req: SapperRequest
	}
) => string

You may call the function multiple times. Transformers will run in the order in which they are registered, with the default Sapper transformer running last.

@happycollision happycollision force-pushed the add-template-transform-api branch 3 times, most recently from 5f72674 to 09f8349 Compare November 22, 2020 05:39
@happycollision
Copy link
Contributor Author

happycollision commented Nov 22, 2020

Wha??? Not sure why this is failing. Works On My Machine™

CleanShot 2020-11-22 at 00 43 14@2x

@happycollision happycollision force-pushed the add-template-transform-api branch 4 times, most recently from 4d44cbb to 87b3c71 Compare November 22, 2020 05:53
@happycollision
Copy link
Contributor Author

Ah. VSCode was tricking me by setting my NODE_ENV to "production" in its terminal. Not sure if I did that somehow years ago or what. 🙄

@benmccann
Copy link
Member

We're working on a successor to Sapper called SvelteKit and I'd rather any major changes go there given our limited review time. The code for it is still private at the moment while we work on getting things in a more stable state, but hopefully we'll have more to share before too long.

@happycollision
Copy link
Contributor Author

Yeah, I was hoping that this was simple enough to be included as a feature in both places (while we wait for SvelteKit). If not, I’ll be happy to propose this same thing again if there are no comparable features.

@happycollision
Copy link
Contributor Author

happycollision commented Nov 22, 2020

Oh, and though the documentation here suggests “major” due to the amount of explanation added, the actual amount of effort and change to the code base is pretty small.

@benmccann
Copy link
Member

This would be a complicated PR to add to Sapper because we have to compare the multiple proposed APIs and determine which if any we should add. This feature has also been proposed in #1152 and #1037

@happycollision happycollision force-pushed the add-template-transform-api branch 3 times, most recently from 67599a0 to 403589b Compare December 31, 2020 19:13
@happycollision happycollision force-pushed the add-template-transform-api branch from 403589b to 332062c Compare December 31, 2020 19:32
@happycollision
Copy link
Contributor Author

@benmccann, here are a few things to consider when comparing the proposed PRs.

I've been comparing the proposed solutions and of the three proposals, there are two levels of functionality: coupled to %sapper.key% and completely arbitrary. #1037 is coupled and both this PR and #1152 are arbitrary. I would argue that arbitrary is the way to go. It is a more complete escape hatch for doing anything you'd want.

So comparing #1152 and this PR reveals the following differences:

This one introduces a new top-level api inside Sapper itself with a function that is used to work with the interface, whereas #1152 hooks into the middleware and provides its new api via the res argument inside any middleware function. It looks as though both can ultimately achieve the same changes, so anything you could do with one should be possible with the other. So the main difference between the two is how we'd prefer the api to work.

A direct comparison of the same change to the template file

PR #1642 version

import polka from 'polka';
import * as sapper from '@sapper/server';

import { start } from '../../common.js';

const app = polka()
+	.use((req, res, next) => {
+		if (req.headers['disable-js'] === 'true') {
+			res.replacers = [(body) => body.replace('%sapper.scripts%', '')]
+		}
+		next()
+	})
	.use(sapper.middleware())

start(app);

This PR version

import polka from 'polka';
import * as sapper from '@sapper/server';

import { start } from '../../common.js';

+sapper.registerTemplateTransformer((template, data) => 
+	template
+	.replace(
+		'%sapper.scripts%',
+		() => data.req.headers['disable-js'] === 'true' ? '': '%sapper.scripts%'
+	)
+
+);

const app = polka()
	.use(sapper.middleware());

start(app);

Of note in the version in this PR, you actually have access to the same data as Sapper itself when doing your replacements. The data argument contains the following:

export type TransformData = Readonly<{
	html: any;
	head: any;
	styles: string;
	script: string;
	nonce_value: string;
	nonce_attr: string;
	req: SapperRequest;
}>;

And Sapper itself uses that exact same data object to do the default transforms. So you have the ability to make decisions based on what Sapper would be doing as well. Which means I could have just as easily done this in the example above

sapper.registerTemplateTransformer((template, data) => 
	template
	.replace(
		'%sapper.scripts%',
-		() => data.req.headers['disable-js'] === 'true' ? '':  '%sapper.scripts%'
+		() => data.req.headers['disable-js'] === 'true' ? '': data.script
	)

);

Whether ultimately extending the res argument in middleware (which I guess could have potential side effects in some code that may interact dynamically with that object?), or providing a top-level api for the transformations, the one thing missing from @arve0's approach that I'd love to see preserved is the direct access to the values that Sapper also uses for default replacements.

Finally, one other subtle difference between the two that may honestly never matter: Using the middleware intrinsically ties this feature to the use of the server itself. Though my implementation does work inside sapper/server, there isn't any reason that the developer-facing api would ever have to change if the use of polka is ever rethought, etc.

@benmccann
Copy link
Member

Thanks! It's helpful to have a comparison between the various PRs.

Right now major new features are on hold as we work on SvelteKit. This might be handled differently there, so I'd be hesitant to introduce something that immediately has to change in the new codebase or that users would have to update while migrating.

@pixelmund
Copy link

Is this still considered for svelte kit? Can't find anything in the milestones or issues about it would be pretty nice to have this feature

@benmccann
Copy link
Member

Thanks for this PR!! I don't think we'll be adding anything new to Sapper at this point, but this now exists in SvelteKit! sveltejs/kit#670

@benmccann benmccann closed this Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ability to replace text on template through middleware Cache busting with url versioning params
3 participants