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 API for idempotent page description editing #1765

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

ivanbakel
Copy link
Contributor

@ivanbakel ivanbakel commented Apr 20, 2022

setDescription and setDescriptionI are slightly surprising in that they are not idempotent, and multiple calls to these functions will insert multiple <meta> tags in the page head. Since these functions present like setTitle and setTitleI, I would have expected them to have a similar idempotent behaviour.

This adds idempotent versions of these functions, which behave like setTitle and setTitleI in that they store information in the page data and can be called any number of times, taking the final value supplied.

Because they use the page data, these functions can't be drop-in replacements for the existing functions - users with custom layouts will have to manually edit them to use the new idempotent functions, since they have to explicitly include the new pageDescription field in their layouts (while the old functions write to the page head.)

Points for review

  • Is promoting description as a meta tag to something stored in the page data sensible? Should the same approach be taken for other meta tags? Should arbitrary meta tags appear in the page data?
  • Is using the page data a sensible place to store this kind of data? Should the per-request cache be used instead?

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Like setTitle, this function should really be idempotent so developers
don't add multiple conflicting meta descriptions to the page. Unlike
setTitle, the function currently fails its idempotency test.
`setDescription` and `setDescriptionI` present a similar API to
`setTitle` and `setTitleI`, but unlike those functions the description
functions are not idempotent - so calling them multiple times inserts
multiple `<meta/>` tags in HTML `<head/>`.

This adds explicitly idempotent versions of those functions which are
handled in a similar way to the title, so that calling them multiple
times has the effect of taking the final value specified.

Because the non-idempotent behaviour of setDescription is not obvious,
this also adds warnings for that behaviour to make it clear what the
effect of multiple calls will be. Unfortunately, setDescriptionIdemp
can't be made a drop-in replacement because developers may have defined
their own layouts which need to take pageDescription into account.
@ivanbakel ivanbakel changed the title Idempotent description Add API for idempotent page description editing Apr 20, 2022
Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! And great docs!

@snoyberg snoyberg merged commit f338e51 into yesodweb:master Apr 21, 2022
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.

2 participants