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 google tagmanager plugin #1123

Merged
merged 11 commits into from
Jun 21, 2017
Merged

Add google tagmanager plugin #1123

merged 11 commits into from
Jun 21, 2017

Conversation

0x80
Copy link
Contributor

@0x80 0x80 commented Jun 7, 2017

In the docs for tagmanager they suggested to inject one of the code snippets directly after the body opening tag. For this reason I add the preBodyComponents API.

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 7, 2017

Deploy preview ready!

Built with commit 5468ce3

https://deploy-preview-1123--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 7, 2017

Deploy preview ready!

Built with commit 5468ce3

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

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 7, 2017

Deploy preview ready!

Built with commit 5468ce3

https://deploy-preview-1123--gatsbyjs.netlify.com

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Nice!

If preBodyComponents is going to be a thing we need to update example sites as well. With #1121 it might make sense to have <Body/> actually have <body><PreBodyComponents /><PageBody /><PostBodyComponents /></body> inside it.

@@ -0,0 +1,14 @@
{
"presets": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a separate babelrc? Ideally every plugin uses the central one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mindless copy-paste from the google analytics plugin :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, Kyle! Why did you include a babelrc there??

At some point we need to just centrally manage all the npmignore, gitignore, etc. files.

You mind removing both then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I just remembered that I put it there to fix a bug! Client side code needs it because otherwise it wont work in older browsers. The backticks will end up in the production bundle for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think ideally we want (especially) these plugins to export code in a way that the user build process can transpile them together with the user code. I was reading this on the subject recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this light I have some new ideas about plugin package structure as well. Because there are standards to distinguish between browser and node parts of a package, I figured that if we can merge the gatsby-ssr and gatsby-node api's in a single entry (since they are both node context), we can use the same technique as other packages use for making the distinction.

You would basically have a main field pointing to the merged gatsby-ssr & gatsby-node entry, and a browser field pointing to the gatsby-browser entry.

Then in the future when webpack supports subfields in esnext you can have esnext.browser point to the raw gatsby-browser code etc. So then plugin packages can export transpiled code and raw code for both contexts at the same time like this:

 "main": "dist/gatsby-node-and-ssr.js",
 "browser: "dist/gatsby-browser.js",
 "esnext": {
      "main": "src/gatsby-node-and-ssr.js",
      "browser": "src/gatsby-browser.js"
 }

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm yeah you're right. That is what we should do. We do after all control the builds so this would be easy to setup. We'd just ship the source code and then users could configure their browserlist according to the needs of their project https://github.com/ai/browserslist

Copy link
Contributor

Choose a reason for hiding this comment

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

gatsby-ssr is not a node environment. It's compiled by webpack and can include webpack stuff like importing files etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see.

@@ -123,14 +130,14 @@ module.exports = (locals, callback) => {
// ignore
}

const dascripts = [
const scripts = [
Copy link
Contributor

Choose a reason for hiding this comment

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

haha you changed my funny name! Ok... fine, time to go corporate I guess ;-)

@KyleAMathews
Copy link
Contributor


setPreBodyComponents([
<script
dangerouslySetInnerHTML={{
Copy link
Contributor

@jbolda jbolda Jun 8, 2017

Choose a reason for hiding this comment

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

@0x80 , Is there a reason you didn't just use the react iframe?

           <noscript>
                <iframe
                  src={'https://www.googletagmanager.com/ns.html?id=XXXX'}
                  height={0}
                  width={0}
                  style={{ display: 'none', visibility: 'hidden'}}>
                </iframe>
              </noscript>

Copy link
Contributor Author

@0x80 0x80 Jun 8, 2017

Choose a reason for hiding this comment

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

@jbolda Hmm yeah it doesn't make sense to put a noscript tag inside a script tag either does it? It's meant to target environments that don't support the script tag 😅

I'll add the noscript part directly to the preBodyComponents.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, in the grand scheme of things, a reactjs site probably won't work for those browsers which would use this anyways! Good to be thorough I guess.

Copy link
Contributor Author

@0x80 0x80 Jun 9, 2017

Choose a reason for hiding this comment

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

Ugh I wish I'd given this thing a bit more thought. I think you're right. Does it make sense to add this if the rest of the website if not going to load without script support?

But maybe it is still useful for analytics to find out how many people land on your website without script support?

@KyleAMathews what do you think?

And if I take this out, should I leave the preBodyComponents API or ditch that too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well... Gatsby is a static site generator. You could think of the React part as a (very) elaborate optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KyleAMathews But all navigation would be broken right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Links are still links :-)

The only thing that'd be broken is js driven stuff like animations or event handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm surprised. I thought site navigation works via react-router and doing history.push() and such?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure but that's an optimization again. W/o JavaScript intercepting clicks, they're just a elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes of course!

@0x80
Copy link
Contributor Author

0x80 commented Jun 8, 2017

@KyleAMathews Do we really need to update the examples for this? I think if a project doesn't use preBodyComponents there is not much use adding it to the html.js. I'd say updating the docs would be enough.

@KyleAMathews
Copy link
Contributor

Eh we're fine I guess yeah not updating examples. I think I do want to do the <Body/> idea so the pre body stuff will just be hidden in there.

@0x80
Copy link
Contributor Author

0x80 commented Jun 9, 2017

@KyleAMathews I am wrapping this up. I forgot to add the API to the develop.js do doing that now. I noticed that develop.js doesn't have an api runner for replaceRenderer (in contrary to static-entry.js). Is that correct?

@0x80
Copy link
Contributor Author

0x80 commented Jun 9, 2017

I can't get this to work. The preBodyComponents I'm adding don't get rendered somehow. Do you see anything wrong?

Also when I try to test locally by adding a gatsby-ssr.js to my project with this content

const React = require('react')

exports.onRenderBody = ({ setPreBodyComponents }) => {
  return setPreBodyComponents([
    <div>
      PRE BODY COMPONENT
    </div>,
  ])
}

I get the error

/Users/thijskoerselman/Development/vauxlab/gatsby-ssr.js:5
    <div>
    ^

SyntaxError: Unexpected token <

@KyleAMathews
Copy link
Contributor

So the error there is because gatsby-* files in sites aren't being processed by Babel/Webpack during development. They are for production but not for development.

@KyleAMathews
Copy link
Contributor

@0x80 still want to push this across the finish line?

@0x80
Copy link
Contributor Author

0x80 commented Jun 15, 2017

@KyleAMathews Yes definitely. I got a bit stuck because the preBodyComponents API didn't seem to be working, but I'll look at it again with a clear head soon.

0x80 added 2 commits June 20, 2017 20:06
# Conflicts:
#	packages/gatsby/src/cache-dir/static-entry.js
@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jun 20, 2017

Deploy preview failed.

Built with commit 5468ce3

https://app.netlify.com/sites/using-contentful/deploys/594a2c95cf321c78b77301d8

@0x80
Copy link
Contributor Author

0x80 commented Jun 20, 2017

@KyleAMathews The API works, but I can't figure out how to render an iframe in a noscript tag. React keeps rendering the iframe part as a string. Any idea how to get around that?

This is what I have:

setPreBodyComponents([
      <noscript
        dangerouslySetInnerHTML={{
          __html: stripIndent`
            <iframe
              src="https://www.googletagmanager.com/ns.html?id=${pluginOptions.id}"
              height="0"
              width="0"
              style="display: none; visibility: hidden"
            />`,
        }}
      />,
    ])

Which renders

<noscript>&lt;iframe
  src="https://www.googletagmanager.com/ns.html?id=GTM-MQ69KNJ"
  height="0"
  width="0"
  style="display: none; visibility: hidden"
/&gt;</noscript>

@KyleAMathews
Copy link
Contributor

Deploy preview failed.

Built with commit 61a4d0c

https://app.netlify.com/sites/using-glamor/deploys/5949780b8ebdd92bf90290fc

@KyleAMathews
Copy link
Contributor

Hmmm... put the noscript part in dangerouslySetInnerHTML as well?

@0x80
Copy link
Contributor Author

0x80 commented Jun 21, 2017

I tried that also. Then the noscript tag is rendered as normal, but its content is still a string. I'll do some more research.

@KyleAMathews
Copy link
Contributor

Perhaps it's the strip indent. Also maybe there's weird rules around noscript

@0x80
Copy link
Contributor Author

0x80 commented Jun 21, 2017

😄 It turns out this is just browser / Chrome behavior. If you turn off javascript the noscript tag content is shown as normal markup in the inspector.

@0x80
Copy link
Contributor Author

0x80 commented Jun 21, 2017

@KyleAMathews I think it's ready

@KyleAMathews
Copy link
Contributor

Hahaha, why is my code behaving exactly as it's supposed to???? :D

Cool, thanks! This is very nicely executed.

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.

4 participants