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

refactor(ganalytics, gtag): move options out of themeConfig #5832

Merged
merged 7 commits into from
Nov 10, 2021

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Oct 30, 2021

Breaking changes

  • For anyone who uses plugin-google-analytics or plugin-google-gtag: you would need to move your googleAnalytics or gtag options from themeConfig to preset-classic configuration.
// docusaurus.config.js

module.exports = {
  presets: [
    [
      '@docusaurus/preset-classic',
      {
        docs: /* ... */,
+       gtag: {
+         trackingID: 'xxx',
+       },
+       googleAnalytics: {
+         trackingID: 'xxx',
+       },
      },
    ],
  ],
  themeConfig: {
-   gtag: {
-     trackingID: 'xxx',
-   },
-   googleAnalytics: {
-     trackingID: 'xxx',
-   },
    // other options
  },
};

Why?

It has always been very counterintuitive to put the plugin options in theme configuration. It was made so previously due to technical limitations (separation of server data from client data), but now, with a better infrastructure, we can make the migration to make any future user less confused.

Motivation

Fulfill the long-standing todo of moving the options.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Build

@Josh-Cena Josh-Cena added pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: polish This PR adds a very minor behavior improvement that users will enjoy. labels Oct 30, 2021
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 30, 2021
@netlify
Copy link

netlify bot commented Oct 30, 2021

✔️ [V2]

🔨 Explore the source changes: 9aadb5e

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/618ba37ec4da26000765255f

😎 Browse the preview: https://deploy-preview-5832--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Oct 30, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 94
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5832--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena force-pushed the jc/give-plugins-their-options-back branch from a6f0cd7 to 697c4b7 Compare October 30, 2021 04:44
@github-actions
Copy link

github-actions bot commented Oct 30, 2021

Size Change: +326 B (0%)

Total Size: 882 kB

Filename Size Change
website/.docusaurus/globalData.json 38.3 kB +153 B (0%)
website/build/assets/js/main.********.js 475 kB +174 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/build/assets/css/styles.********.css 94.2 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 66.1 kB 0 B
website/build/blog/index.html 36.8 kB 0 B
website/build/docs/index.html 44 kB +1 B (0%)
website/build/docs/installation/index.html 51.5 kB -2 B (0%)
website/build/index.html 29.5 kB 0 B
website/build/tests/docs/index.html 25.2 kB 0 B
website/build/tests/docs/standalone/index.html 21.7 kB 0 B

compressed-size-action

@Josh-Cena Josh-Cena marked this pull request as draft October 30, 2021 06:32
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM, why still draft?

I suggest adding some edits in this doc page too: https://docusaurus.io/docs/presets#docusauruspreset-classic

@@ -22,7 +22,7 @@ declare module '@generated/site-metadata' {
import type {DocusaurusSiteMetadata} from '@docusaurus/types';

const siteMetadata: DocusaurusSiteMetadata;
export default siteMetadata;
export = siteMetadata;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure to understand what's the advantage of this syntax?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is about how you're able to import it. Because we have esModuleInterop there's not a visible change. I forgot why I did this change but at least now it's "more correct". export = X is for module.exports = X and export default X is for module.exports.default = X.

@Josh-Cena
Copy link
Collaborator Author

LGTM, why still draft?

I actually don't know if my changes are correct because I don't know how to test them... Any ideas?

@slorber
Copy link
Collaborator

slorber commented Nov 4, 2021

If you use the Docusaurus prod site locally it should send requests to GA when navigating, if requests are 200 then we can assume it's working

@Josh-Cena
Copy link
Collaborator Author

I'll take this as a success 👀

image

@Josh-Cena Josh-Cena force-pushed the jc/give-plugins-their-options-back branch from fcc0c79 to 99da7a6 Compare November 7, 2021 04:09
@Josh-Cena Josh-Cena marked this pull request as ready for review November 7, 2021 04:47
@slorber
Copy link
Collaborator

slorber commented Nov 10, 2021

LGTM thanks 👍

@Josh-Cena Josh-Cena merged commit ac88d97 into main Nov 10, 2021
@Josh-Cena Josh-Cena deleted the jc/give-plugins-their-options-back branch November 10, 2021 11:04
@MateuszDabrowski
Copy link

I don't get the argument behind the change, but what is not counterintuitive is having gtag/ganalytics configuration in a completely different place from algolia configuration.

Also, if you believe that "counterintuitive to put the plugin options in theme configuration" - why not add it as a separate element of the plugins section? Moving it into a specific preset is just as (if not more) counterintuitive to me.

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Dec 12, 2021

Algolia is a theme (theme-search-algolia); the google ones are plugins (plugin-google-analytics). It's quite intuitive for themes to have their configuration in the theme config, less so for plugins to have their options in the theme config as well. (In fact, we are considering the possibility of getting rid of the customCss option for theme-classic: #3678 after that, none of the themes would have any options. That seems to be the most natural API design.)

If you had previously been unaware of the nuance between themes and plugins and just put all of them under plugins, now is a good chance to try out the new shorthand configuration to actually make yourself aware of that.

As a regular helper on Discord, you can't imagine the confusion from newcomers. I've seen people declaring themeConfig multiple times because they just rigidly followed the instructions without even realizing there's already an entry called themeConfig. I've seen them putting themeConfig in the place where they are supposed to put plugin options. It just appears much more natural to co-locate plugins with their options.

why not add it as a separate element of the plugins section?

I have no idea what you meant. Preset options are directly passed down to individual plugins. When you write:

module.exports = {
  presets: [
    [
      '@docusaurus/preset-classic',
      {
        googleAnalytics: {
          trackingID: 'UA-141789564-1',
          anonymizeIP: true,
        },
      },
    ],
  ],
};

It's identical in every way to:

module.exports = {
  plugins: [
    [
      '@docusaurus/plugin-google-analytics',
      {
        trackingID: 'UA-141789564-1',
        anonymizeIP: true,
      },
    ],
  ],
};

You are welcome to use an individual plugin as opposed to the preset if the latter makes more sense to you.

szczys added a commit to golioth/docs that referenced this pull request Feb 7, 2022
szczys added a commit to golioth/docs that referenced this pull request Feb 7, 2022
pabloest added a commit to semgrep/semgrep-docs that referenced this pull request Feb 17, 2022
pabloest added a commit to semgrep/semgrep-docs that referenced this pull request Feb 18, 2022
* Upgrade Docusaurus (and plugins) to 2.0.0-beta.15

* Move gtag config out of themeConfig and into preset-classic config

Per: facebook/docusaurus#5832
drarmstr added a commit to bsouthga/Recoil that referenced this pull request Feb 18, 2022
pixelb added a commit to pixelb/hydra that referenced this pull request Feb 18, 2022
Note the following does not update to the latest beta docusaurus
  $ yarn upgrade docusaurus docusaurus-plugin-internaldocs-fb --latest
Therefore manually set "2.0.0-beta.15" package.json and rerun `yarn upgrade`

The docusaurus.config.js change is required in the new version due to:
facebook/docusaurus#5832

The website/src/components/GithubLink.jsx change is required due to:
facebook/docusaurus#6516
SallyMcGrath added a commit to CodeYourFuture/syllabus that referenced this pull request Mar 1, 2022
facebook-github-bot pushed a commit to facebook/metro that referenced this pull request Mar 4, 2022
Summary:
Update docusaurus to 2.0.0-beta.16

- Moves the `gtag` config, following:
```
[ERROR] Error: The "gtag" field in themeConfig should now be specified as option for plugin-google-gtag. For preset-classic, simply move themeConfig.gtag to preset options. More information at facebook/docusaurus#5832.
```

- Remove our dependence on `react-markdown` (only used for the Help page) to resolve a build error:
```
Module not found: Error: Can't resolve 'path' in '/Users/rob/workspace/metro/website/node_modules/react-markdown/node_modules/vfile'
BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "path": require.resolve("path-browserify") }'
	- install 'path-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "path": false }
Module not found: Error: Can't resolve 'path' in '/Users/rob/workspace/metro/website/node_modules/replace-ext'
BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "path": require.resolve("path-browserify") }'
	- install 'path-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "path": false }
```

- Sets `isCloseable: false` (a new docusaurus feature) on our Ukraine banner, in line with React Native

Reviewed By: motiz88

Differential Revision: D34614027

fbshipit-source-id: 15a014ac5c821c01e39fa4df9365e1886e2642fb
kyletgoldberg added a commit to facebookexperimental/Robyn that referenced this pull request Mar 23, 2022
EnigmoPantera added a commit to autopi-io/documentation that referenced this pull request May 23, 2022
Replace the "Metadatas" keyword with "Metadata" facebook/docusaurus#5871
Moved gtag options out of themeConfig facebook/docusaurus#5832
ericnakagawa added a commit to ericnakagawa/docs-1 that referenced this pull request Jun 24, 2022
rscarrasco added a commit to rscarrasco/apiato-documentation that referenced this pull request Nov 1, 2022
Docusaurus has issued a breaking change in which both `googleAnalytics`
and `gtag` plugins should be moved out of the `themeConfig`. This commit
does just that.

See: facebook/docusaurus#5832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants