Warn when implementing apis twice #3889

m-allanson · 2018-02-07T13:11:43Z

This has come up a whole bunch of times, see #2005, #2963, #3231, #2578, #1079 (there might be more).

Now when implementing replaceRenderer multiple times, you'll see a warning in the console.

Still to do:

~~The warning is output multiple times, which is a bit messy, but an improvement on no output.~~

~~Add a dedicated docs page demonstrating how to fix this. For now it points to @akadop's comment here~~

~~Ensure local gatsby-ssr.js takes precedence when replaceRenderer is implemented in multiple places~~

~~Use gatsby-cli's reporter for nicer output formatting?~~

gatsbybot · 2018-02-07T13:17:37Z

Deploy preview for gatsbygram ready!

Built with commit db3e56d

https://deploy-preview-3889--gatsbygram.netlify.com

KyleAMathews

Sweeeeeet!!! This is so needed. Thanks for taking this on!

KyleAMathews · 2018-02-07T18:40:56Z

packages/gatsby/cache-dir/api-runner-ssr.js

+}
+
+// Run the specified api in any plugins that have implemented it
+const apiRunner = ({ api, args, defaultReturn, checkDupes = false }) => {


I like named arguments (should have used them here originally) but we'd need to update the node/browser api runners as well to keep things consistent internally. So could you revert this change for now and perhaps do the named args change in another PR?

KyleAMathews · 2018-02-07T18:41:53Z

packages/gatsby/cache-dir/api-runner-ssr.js

+    const dupes = duplicatedApis(plugins, api)
+    if (dupes.length > 0) {
+      let m = `\nThe "${api}" api has been implemented multiple times. Only the last implementation will be used.`
+      let m2 = `This is probably an error, see https://github.com/gatsbyjs/gatsby/issues/2005#issuecomment-326787567 for workarounds.`


This should be a docs page — could you add a new docs page explaining the problem and solutions and link to that instead?

Oh, you already said you were going to do this :-)

KyleAMathews · 2018-02-07T18:43:51Z

packages/gatsby/cache-dir/api-runner-ssr.js

+
+  if (checkDupes) {
+    const dupes = duplicatedApis(plugins, api)
+    if (dupes.length > 0) {


It looks like you need to filter this list to APIs where duplicates is a problem e.g. replaceRenderer. Most SSR APIs are fine.

Also, I think we should suppress warnings for duplicated APIs where there's an implementation in the site's gatsby-* file as those always override other plugins. This is the normal fix — merge plugin's implementations of replaceRenderer. As things are now, the warning would keep showing which isn't what we want as then the user might uninstall the plugin to fix things but which would actually probably break things as many plugins with gatsby-ssr.js files also have other API implementations e.g. for browser APIs.

It looks like you need to filter this list to APIs where duplicates is a problem

This is handled at the point that apiRunner is called, using the new checkDupes flag:

// check for duplicate api implementations apiRunner({api: `replaceRenderer`, args, checkDupes: true}) // don't check for duplicate api implementations (default behaviour) apiRunner({api: `replaceRenderer`, args})

But actually it's just the replaceRenderer api that shouldn't be duplicated? For now I can hardcode the name replaceRenderer within apiRunner. Then we can extract it out later if necessary.

we should suppress warnings for duplicated APIs where there's an implementation in the site's gatsby-* file

👍

m-allanson · 2018-02-08T11:55:56Z

@KyleAMathews thinking a bit more about your comment on when to show an error...

we should suppress warnings for duplicated APIs where there's an implementation in the site's gatsby-* file

Is it true that - when someone has a couple of replaceRenderer plugins installed and also has replaceRenderer in their gatsby-ssr.js, we have no way of knowing if that's intentional or accidental?

The different states that we can detect are like this:

Status	replaceRenderer implementations
Error	in plugin A, in plugin B
Possible error	in plugin A, in plugin B, in local gatsby-ssr.js
No error	in plugin C
No error	in local gatsby-ssr.js

When there's definitely an error, we can show an error message. When there's possibly an error, what should Gatsby do? I guess the options are:

Show the standard error message (with link to docs page)
Show a shorter "heads up" message pointing to the docs page
Stay silent
Something else?

m-allanson · 2018-02-08T15:25:51Z

@KyleAMathews I made a first draft at some docs, added to this PR. I also created an example repo to check what I was writing as I went along.

In that example repo, the final commit should result in a functioning gatsby build, but that's not happening. The installed plugin is taking precedence over the local gatsby-ssr.js. Is that a bug?

KyleAMathews · 2018-02-08T18:11:24Z

docs/docs/debugging-replace-renderer-api.md

+
+## What is the `replaceRenderer` API?
+
+The `replaceRenderer` API is one of [Gatsby's Server Side Rendering (SSR) hooks](https://www.gatsbyjs.org/docs/ssr-apis/#replaceRenderer). It's used to customise how Gatsby renders your static content. It can be implemented by any Gatsby plugin or your `gatsby-ssr.js` file - adding support for Redux, CSS-in-JS libraries or any code that needs to change Gatsby's default HTML output.


do a relative link so gatsby-plugin-catch-links can do its thing

I'd also immediately make it clear this stuff is all involved when running gatsby build. E.g. the second sentence is "This API is called when you run gatsby build"

KyleAMathews · 2018-02-08T18:12:41Z

docs/docs/debugging-replace-renderer-api.md

+
+## Why does it cause build errors?
+
+When using `replaceRenderer` many times in your project, only the newest instance will be used - which can cause unexpected problems with your site.


s/will/can/

"only the last plugin implementing the API can be called"

KyleAMathews · 2018-02-08T18:14:46Z

docs/docs/debugging-replace-renderer-api.md

+If your project uses replaceRenderer more than once, gatsby build will warn you:
+
+```
+The "replaceRenderer" api has been implemented multiple times. Only the last implementation will be used.


Make sure the actor is mentioned i.e. plugin. So the sentence could read 'The "replaceRenderer" API is implemented by several enabled plugins."

KyleAMathews · 2018-02-08T18:15:32Z

docs/docs/debugging-replace-renderer-api.md

+The "replaceRenderer" api has been implemented multiple times. Only the last implementation will be used.
+This is probably an error, see https://example.com for workarounds.
+Check the following files for "replaceRenderer" implementations:
+/path/to/my/site/node_modules/gatsby-plugin-styled-components/gatsby-ssr.js


I'd list the plugin name first (as that's what will be recognizable" and then list the exact path to the file implementing the API.

What's the best way to get the plugin name? Just extract it from the path?

We could move the duplicate plugin detection to when we load the plugins. There we have access to all the info we need. Then when we write out the SSR runner, we just specify which plugin's implementation of replaceRenderer it should run which is all that file needs to know.

KyleAMathews · 2018-02-08T18:17:11Z

docs/docs/debugging-replace-renderer-api.md

+
+In this example, your `gatsby-ssr.js` file and `gatsby-plugin-styled-components` are both using `replaceRenderer`.
+
+### 2. Move their `replaceRenderer` functionality to your `gatsby-ssr.js` file


"Copy the plugins' replaceRenderer functionality to your site's gatsby-ssr.js file"

KyleAMathews · 2018-02-08T18:18:24Z

docs/docs/debugging-replace-renderer-api.md

+
+### Initial setup
+
+In this example project you're using [`redux`](https://github.com/gatsbyjs/gatsby/tree/master/examples/using-redux) and [Gatsby's Styled Components plugin](https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-plugin-styled-components).


Say "we" instead of "you"

KyleAMathews · 2018-02-08T18:20:44Z

packages/gatsby/cache-dir/api-runner-ssr.js

@@ -6,10 +6,45 @@

 const apis = require(`./api-ssr-docs`)

-module.exports = (api, args, defaultReturn) => {
+/**
+ * Some apis should only be implemented once. Given a list of plugins, and an


Capitalize API

KyleAMathews · 2018-02-08T18:24:08Z

Show a shorter "heads up" message pointing to the docs page

Yeah, that makes sense. The user probably has things handled at that point but it's still useful to warn people e.g. they add a custom replaceRenderer but then add later another plugin with a replaceRenderer. When at that point they're wondering why things aren't working, they'll have the console message to hopefully point them in the right direction.

KyleAMathews · 2018-02-08T18:28:20Z

In that example repo, the final commit should result in a functioning gatsby build, but that's not happening. The installed plugin is taking precedence over the local gatsby-ssr.js. Is that a bug?

Yes... that would be a bug...

Hmmm not even sure when/where this is implemented. I think it's last one wins but that's kind of confusing as that means we're running code for API implementations we're not using.

We should add the APIs here which can only be run once and then explicitly choose just the last one to run https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/api-runner-ssr.js

m-allanson · 2018-02-08T21:13:36Z

That's great docs feedback, thanks! PR updated to cover that. Tomorrow I'll take another look at the functionality side of things.

Note to self - update error message to mention plugin name.

KyleAMathews

More copy tweaks

KyleAMathews · 2018-02-08T23:05:05Z

docs/docs/debugging-replace-renderer-api.md

+
+## What is the `replaceRenderer` API?
+
+The `replaceRenderer` API is one of [Gatsby's Server Side Rendering (SSR) hooks](/docs/ssr-apis/#replaceRenderer). This API is called when you run `gatsby build` and is used to customise how Gatsby renders your static content. It can be implemented by any Gatsby plugin or your `gatsby-ssr.js` file - adding support for Redux, CSS-in-JS libraries or any code that needs to change Gatsby's default HTML output.


Should be called "extension APIs" instead of hooks https://www.gatsbyjs.org/docs/api-specification/#extension-apis

KyleAMathews · 2018-02-08T23:06:04Z

docs/docs/debugging-replace-renderer-api.md

+
+## Why does it cause build errors?
+
+When using `replaceRenderer` many times in your project, only the last plugin implementing the API can be called - which will break your site builds.


"If multiple plugins implement replaceRenderer in your project"

KyleAMathews · 2018-02-08T23:07:27Z

docs/docs/debugging-replace-renderer-api.md

+
+Note that `replaceRenderer` is only used during `gatsby build`. It won't cause problems as you work on your site with `gatsby develop`.
+
+If your project uses `replaceRenderer` more than once, `gatsby build` will warn you:


Make similar change here. The project isn't the actor, plugins are.

KyleAMathews · 2018-02-09T15:31:49Z

Deploy preview for using-glamor failed.

Built with commit 4268d14

https://app.netlify.com/sites/using-glamor/deploys/5a7dbe6948769b76452b9f41

m-allanson · 2018-02-09T16:46:48Z

@KyleAMathews in bootstrap/load-plugins.js, Gatsby checks that plugins only implement known APIs, but it only does this for the Node APIs.

A plugin that implements ssrAPIs won't have those APIs checked, its plugin object will end up looking like this:

{ 
  resolve: '/path/to/site/node_modules/gatsby-plugin-styled-components',
  id: 'Plugin gatsby-plugin-styled-components',
  name: 'gatsby-plugin-styled-components',
  version: '2.0.5',
  pluginOptions: { plugins: [] },
  nodeAPIs: [] 
}

should this object be expanded to include ssrAPIs and browserAPIs keys?

KyleAMathews · 2018-02-09T17:23:55Z

@m-allanson yeah I think so. I'm not entirely sure now why I didn't make it check browser/ssr APIs as well.

m-allanson · 2018-02-17T08:02:51Z

Add link to https://github.com/caki0915/gatsby-starter-redux on the docs page?

m-allanson · 2018-02-17T23:30:58Z

@KyleAMathews I was going to add some tests, but it's late so I'm just going to push this as-is. Here's what the various error outputs look like now:

Develop

Possible error (build continues)

There's three replaceRenderers here, the first two are skipped, leaving only the one in the site's local gatsby-ssr.js.

Definite error (build continues)

There's multiple replaceRenderers and none of them are in the site's local gatsby-ssr.js. This is definitely an error.

Build

Possible error (build continues)

As with develop, show a warning but continue with the build.

Definite error (build exits)

As with develop, show an error but because this is definitely an error - stop the build.

m-allanson · 2018-02-19T14:44:36Z

packages/gatsby/src/bootstrap/load-plugins.js

@@ -10,6 +10,7 @@ const nodeAPIs = require(`../utils/api-node-docs`)
 const browserAPIs = require(`../utils/api-browser-docs`)
 const ssrAPIs = require(`../../cache-dir/api-ssr-docs`)
 const resolveModuleExports = require(`./resolve-module-exports`)
+const reporter = require(`gatsby-cli/lib/reporter`)


Hmm maybe using reporter() here isn't right?

See http://www.hemingwayapp.com/

- remove duplicate error messages - improve message format - only run the last replaceRenderer plugin

m-allanson · 2018-02-26T20:05:34Z

@KyleAMathews The latest commit splits the plugin loader out into smaller pieces and adds some tests, so I think this is ready for another look. See my previous comment ⬆️for examples of the error messages.

KyleAMathews · 2018-02-26T20:34:03Z

packages/gatsby/src/bootstrap/index.js

+  const hasAPIFile = (env, plugin) => {
+    // The plugin loader has disabled SSR APIs for this plugin. Usually due to
+    // multiple implementations of an API that can only be implemented once
+    if (env === `ssr` && plugin.skipSSR === true) return undefined


Should we disable plugins for individual APIs not all SSR APIs?

I implemented disabling individual APIs on my checkout, but then had a thought - is there any reason for a plugin to implement replaceRenderer and onRenderBody? I can't find any official or community plugins that do this.

Disabling all SSR APIs keeps things simpler as it means api-runner-ssr.js doesn't have to check whether it should run an API or not. I'd be tempted to roll with this implementation and then review if it causes problems.

It does seem unlikely. If we add more APIs in the future, this could potentially become not true — that a plugin would implement multiple ssr APIs. I did a query (this one was fun ag -l replaceRenderer | xargs -L 1 ag onRenderBody -l) and there isn't any plugin currently that implements both. So let's go with it and see what happens. It'd be easy to change later.

That sounds good. Nice querying :)

KyleAMathews · 2018-02-27T19:08:28Z

Great stuff! Super glad you took this on! It'll save a lot of people from long head-scratching debugging sessions!

KyleAMathews suggested changes Feb 7, 2018

View reviewed changes

KyleAMathews suggested changes Feb 8, 2018

View reviewed changes

m-allanson force-pushed the topics/warn-duplicate-apis branch from d685018 to e907671 Compare February 8, 2018 21:05

KyleAMathews suggested changes Feb 8, 2018

View reviewed changes

m-allanson force-pushed the topics/warn-duplicate-apis branch from e907671 to 4268d14 Compare February 9, 2018 15:29

This was referenced Feb 12, 2018

Add browserAPIs and ssrAPIs to loaded plugin info #3989

Merged

Plugin loader can't parse local ES6 files #4037

Closed

m-allanson force-pushed the topics/warn-duplicate-apis branch from 4268d14 to 0725c00 Compare February 17, 2018 10:45

ghost assigned m-allanson Feb 17, 2018

ghost added the review label Feb 17, 2018

m-allanson commented Feb 19, 2018

View reviewed changes

m-allanson added 8 commits February 26, 2018 11:13

Warn when replaceRenderer is implemented multiple times

acfe558

Remove unused import

66af1b9

Add tests

959d7f4

Back out 'named arguments' change from apiRunner

5dae156

Re-add required import

c577de6

Add first draft of docs page

881f066

Simplify language based on results from Hemingway App

e9de6c4

See http://www.hemingwayapp.com/

Implement docs feedback

6298c41

m-allanson added 7 commits February 26, 2018 11:13

Capitalisation

44c30a4

Copy tweaks

3913207

More concise 'add internal plugins'

00f39e7

Use work from previous PR's to improve this

55b7d3f

- remove duplicate error messages - improve message format - only run the last replaceRenderer plugin

Remove unused import

360677b

Use the reporter for nicer error formatting

57375ee

Exit on error during build

68cfa35

m-allanson force-pushed the topics/warn-duplicate-apis branch from c8b6e47 to 19a1947 Compare February 26, 2018 17:54

Split code out into smaller functions and add tests

a4807ee

m-allanson force-pushed the topics/warn-duplicate-apis branch from 19a1947 to a4807ee Compare February 26, 2018 19:55

KyleAMathews reviewed Feb 26, 2018

View reviewed changes

Update comments

db3e56d

KyleAMathews approved these changes Feb 27, 2018

View reviewed changes

KyleAMathews merged commit aa14992 into gatsbyjs:master Feb 27, 2018

ghost removed the review label Feb 27, 2018

KyleAMathews mentioned this pull request Feb 28, 2018

No longer able to find local plugin after upgrading Gatsby #4274

Closed

m-allanson deleted the topics/warn-duplicate-apis branch February 28, 2018 08:23

This was referenced Feb 28, 2018

Publish replaceRenderer docs #4278

Merged

Warn when user has multiple plugins implementing APIs that should only have 1 #2005

Closed

gatsby-emotion-plugin and Redux don't work together on production builds #2578

Closed

pieh mentioned this pull request Feb 28, 2018

Fix building on windows #4280

Merged

m-allanson mentioned this pull request Mar 16, 2018

local plugin resolution broken when plugin uses gatsby-browser.js but exposes exports via es6 export... #4571

Closed


		## What is the `replaceRenderer` API?

		The `replaceRenderer` API is one of [Gatsby's Server Side Rendering (SSR) hooks](https://www.gatsbyjs.org/docs/ssr-apis/#replaceRenderer). It's used to customise how Gatsby renders your static content. It can be implemented by any Gatsby plugin or your `gatsby-ssr.js` file - adding support for Redux, CSS-in-JS libraries or any code that needs to change Gatsby's default HTML output.


		## Why does it cause build errors?

		When using `replaceRenderer` many times in your project, only the newest instance will be used - which can cause unexpected problems with your site.


		In this example, your `gatsby-ssr.js` file and `gatsby-plugin-styled-components` are both using `replaceRenderer`.

		### 2. Move their `replaceRenderer` functionality to your `gatsby-ssr.js` file


		### Initial setup

		In this example project you're using [`redux`](https://github.com/gatsbyjs/gatsby/tree/master/examples/using-redux) and [Gatsby's Styled Components plugin](https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-plugin-styled-components).


		Note that `replaceRenderer` is only used during `gatsby build`. It won't cause problems as you work on your site with `gatsby develop`.

		If your project uses `replaceRenderer` more than once, `gatsby build` will warn you:

Warn when implementing apis twice #3889

Warn when implementing apis twice #3889

Conversation

m-allanson commented Feb 7, 2018 • edited Loading

gatsbybot commented Feb 7, 2018 • edited Loading

KyleAMathews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-allanson commented Feb 8, 2018

m-allanson commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleAMathews commented Feb 8, 2018

KyleAMathews commented Feb 8, 2018

m-allanson commented Feb 8, 2018 • edited Loading

KyleAMathews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleAMathews commented Feb 9, 2018

m-allanson commented Feb 9, 2018 • edited Loading

KyleAMathews commented Feb 9, 2018

m-allanson commented Feb 17, 2018

m-allanson commented Feb 17, 2018 • edited Loading

Develop

Build

Choose a reason for hiding this comment

m-allanson commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleAMathews commented Feb 27, 2018

m-allanson commented Feb 7, 2018 •

edited

Loading

gatsbybot commented Feb 7, 2018 •

edited

Loading

m-allanson commented Feb 8, 2018 •

edited

Loading

m-allanson commented Feb 8, 2018 •

edited

Loading

m-allanson commented Feb 9, 2018 •

edited

Loading

m-allanson commented Feb 17, 2018 •

edited

Loading