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

Added options.preprocess and tests #1001

Closed
wants to merge 1 commit into from
Closed

Added options.preprocess and tests #1001

wants to merge 1 commit into from

Conversation

DPr00f
Copy link

@DPr00f DPr00f commented Nov 24, 2019

Added options.preprocess and tests

Description

Currently you're not allowed to replace/add custom data to the generated html.

Issues

Since a developer can't replace values in the template.html they'll end up with hacky solutions when wanting to replace values onto the generated html.

This is specially important for localisation and if the developer wants to replace <html lang="%lang%"> in the template itself.

Changes

  • Added a custom option to the sapper.middleware called preprocess
  • preprocess function gets called before the status and the response is ended
  • Added tests called preprocess that illustrate a replacement

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

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@DPr00f
Copy link
Author

DPr00f commented Nov 24, 2019

As a side note to this PR I understand many assumptions and a lot of liberty was taken.

I just want this discussion to not be ignored as it's important for i18n.

Possibly this could be done like the nonces option is done as well or with a middleware system for the sapper.middleware.

Just thought of preprocess for the name of the property as the whole project is surrounded by preprocessers.

Also, would like to point out that this can now be achieved with post-processing but it feels wrong IMHO.

Other possible solutions and I would be happy to contribute to them:

Add variables to the locals

Adding to res.locals.sapper object.

i.e.

// template.html
...
<html lang="%sapper.lang%">
...

// server.js

...
.use((req, res) => {
  res.locals.sapper = {
    lang: extractLangFromUrl(req.url)
  };
})
...

Middlewares

// server.js
...
.use(
  sapper.middleware({
    middleware: [
      (body, req, res) => body.replace("%sapper.lang%", extractLangFromUrl('req.url')),
      otherMiddleware()
    ]
  })
)

@benmccann benmccann mentioned this pull request May 21, 2020
Closed
@benmccann
Copy link
Member

Thanks for putting together this PR! Sapper never had a good method of handling configuration, which made it difficult to know how to integrate this feature. However, that's been addressed in SvelteKit, so hopefully we can address this there. Someone's already sent a PR for it: sveltejs/kit#641

@benmccann benmccann closed this Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DPr00f @benmccann