Skip to content

Fix plausible script tags and make them configurable #105

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

Merged
merged 13 commits into from
Nov 11, 2022

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Nov 5, 2022

Before, the plausible scripts added by this theme were:

  1. broken in some metrics like bounce rate because the Script tag put them at the bottom.
  2. all hard-coded to point to dvc.org 😬

This PR changes that by turning the plausible domain and script source into options configurable via Gatsby's siteMetadata feature under the keys plausibleDomain and plausibleSrc.

  • plausibleDomain determines the domain the site reports to Plausible as, and if it isn't specified the domain is inferred from siteMetadata.siteUrl

  • plausibleSrc allows us to specify the src tag of the plausible script, defaulting to our websites-server-proxied version of the outbound-links script. Cases where this would be overridden would include:

    • using the non-outbound-links version of the script
    • using a proxy at a path different from websites-server
    • using a non-proxy version

    This will change once we shift toward making this more useful to the public

⚠️ I've rebased this branch onto a new maintenance branch that reverts #33, so we can make a bugfix release on the 0.1 line and make this fix get in ASAP

Here's an example of the resulting script tag
image

@rogermparent rogermparent requested a review from a team November 5, 2022 04:31
@rogermparent rogermparent self-assigned this Nov 5, 2022
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 5, 2022 04:31 Inactive
Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

Thanks for the catch @rogermparent.

We use websites-server on all our websites. The proxy should be available to all the websites themselves i.e we will not need the dvc.org prefix.
For convenience, we can

  1. set the plausibleSrc to /pl/js/script.outbound-links.js or /pl/js/script.js by default,
  2. we will also need plausibleAPI which we can set to /pl/api/event by default,
  3. for plausibleDomain I think we can extract it from siteUrl metadata. Instead, we can replace plausibleDomain with a flag variable(which is set to true by default) and set it to false in case we don't want to use it(for instance, the gatsby-theme-iterative example website).

This way we have fewer configurations to do on consumer websites and we can replace the variable only when it's necessary.

@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 7, 2022 18:10 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 10, 2022 18:51 Inactive
@rogermparent rogermparent changed the base branch from main to maintenance November 10, 2022 18:52
@rogermparent
Copy link
Contributor Author

I'm struggling to find out where we actually put /pl/api/event link, I'm looking over all the existing plausible code but it seems like the only two values used are the domain and src that links import from. In the meantime, I'll add the automagic setup for what this PR has to make the patch more seamless.

@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 10, 2022 19:31 Inactive
Comment on lines 132 to 138
{plausibleDomainOrDefault ? (
<script
defer
data-domain={plausibleDomainOrDefault}
src={plausibleSrc}
/>
) : null}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

falsiness can be triggered with a plausibleDomain of '', but that API will be improved in the next revision.

@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 10, 2022 20:12 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 10, 2022 20:37 Inactive
@rogermparent rogermparent marked this pull request as draft November 10, 2022 20:42
@rogermparent
Copy link
Contributor Author

Found it! It's just data-api on the main script 🤦. Integrating it now.

@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 10, 2022 23:33 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 10, 2022 23:41 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 11, 2022 03:40 Inactive
@rogermparent rogermparent marked this pull request as ready for review November 11, 2022 04:10
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 11, 2022 04:14 Inactive
@@ -172,7 +172,8 @@ module.exports = ({
],
siteMetadata: {
author: 'Iterative',
siteUrl: 'https://cml.dev',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can get away with a lot of plug-and-play for our websites, but I think we shouldn't have a default show through on siteUrl, especially not cml.dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

We provide siteUrl for individual websites, so we don't need a default value. It will be required field for individual website.

@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 11, 2022 04:25 Inactive
Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work @rogermparent.

@rogermparent rogermparent had a problem deploying to gatsby-theme-fix-script-rtuc1d November 11, 2022 04:49 Failure
@rogermparent rogermparent temporarily deployed to gatsby-theme-fix-script-rtuc1d November 11, 2022 05:17 Inactive
@rogermparent rogermparent merged commit 2d3096d into maintenance Nov 11, 2022
@rogermparent rogermparent deleted the fix-script-tags branch November 11, 2022 05:25
rogermparent added a commit that referenced this pull request Nov 11, 2022
* Add plausible through metadata

* Use Gatsby SiteMetadata for plausible domain and src

* Use automatic plausible inference and relative proxy source links

* Disable plausible on example

* v0.1.23

* Add plausibleAPI

* Re-disable plausible in example

* Fix conditional plausible disable

* Re-add defaults for max compatibility

* Use disablePlausible flag and change mechanism to signal with src

* Use pluginOptions for src/api/domain and use src presence over extra boolean flag

* Allow null on plausible options

* Change src to null
rogermparent added a commit that referenced this pull request Nov 14, 2022
…main) (#123)

* Fix plausible script tags and make them configurable (#105)

* Add plausible through metadata

* Use Gatsby SiteMetadata for plausible domain and src

* Use automatic plausible inference and relative proxy source links

* Disable plausible on example

* v0.1.23

* Add plausibleAPI

* Re-disable plausible in example

* Fix conditional plausible disable

* Re-add defaults for max compatibility

* Use disablePlausible flag and change mechanism to signal with src

* Use pluginOptions for src/api/domain and use src presence over extra boolean flag

* Allow null on plausible options

* Change src to null

* Fix gatsby-ssr
@jorgeorpinel
Copy link

Hi. Now that this has been merged everywhere, should we setup some follow-up date to check on the bounce rates?

@rogermparent
Copy link
Contributor Author

I've actually been checking in the whole time! Bounce rates went down immediately after the initial quickfix, and have stayed at that same low/normal level with all other metrics normal ever since the merge. My last check was Monday, and I planned to give yet another Friday for extra measure.

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.

3 participants