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.

Sign up for GitHub

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

Jump to bottom

feat(gatsby-plugin-google-analytics): enable more options. #15280

Merged
merged 5 commits into from
Jul 30, 2019
Merged

feat(gatsby-plugin-google-analytics): enable more options. #15280

merged 5 commits into from
Jul 30, 2019

Conversation

tinder-kyleboss
Copy link
Contributor

@tinder-kyleboss tinder-kyleboss commented Jul 1, 2019 •
edited
Loading

Description

More options for gatsby-plugin-google-analytics

🥁The new options are🥁

  • allowAdFeature
  • dataSource
  • queueTime
  • forceSSL
  • transport

Implementation details

This included breaking out knownOptions into three two factions:

  • createOnly
  • general
    - aLaCarte

❤️ Complete with tests ❤️

Related Issues

#15168

@tinder-kyleboss
Added more plugin options for gatsby-plugin-google-analytics.
3e96a4c 
This included breaking out `knownOptions` into three factions:
- createOnly
- general
- aLaCarte

All options are from general, but I did not add all of the general
options because a few do not work in a set-it-and-forget-it kinda
way.

Added some tests.
@tinder-kyleboss tinder-kyleboss requested a review from a team as a code owner July 1, 2019 17:32
@tinder-kyleboss tinder-kyleboss changed the title Added more plugin options for gatsby-plugin-google-analytics. feat(gatsby-plugin-google-analytics) Added more plugin options for gatsby-plugin-google-analytics. Jul 1, 2019
@tinder-kyleboss tinder-kyleboss changed the title feat(gatsby-plugin-google-analytics) Added more plugin options for gatsby-plugin-google-analytics. feat(gatsby-plugin-google-analytics) Added more options. Jul 1, 2019
@tinder-kyleboss
Copy link
Contributor Author

Would it be possible for someone that has the privileges to re-run the e2e_tests_production_runtime? This is my first contribution so I am not too sure if the end-to-end tests are flakey. The failures do not seem related to the changes made in this PR :) (obvi not ruling that out though!)

@tinder-kyleboss
Copy link
Contributor Author

Looks like it passed :) Thanks fam. 👍

wardpeet
wardpeet reviewed Jul 9, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

This looks really great! Thank you for updating! I added a small comment about the a la carte option.

packages/gatsby-plugin-google-analytics/src/gatsby-ssr.js Outdated
forceSSL: `boolean`,
transport: `string`,
},
aLaCarte: {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you elaborate on why this is added? I can't seem to find this being used in your changes?

Copy link
Contributor Author

@tinder-kyleboss tinder-kyleboss Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Absolutely! Good question 👍

The aLaCarte section is for options that are not directly from the Google Analytics API/docs, but rather options specific to this Gatsby Plugin.

The GA docs/API break all options into sections such as create only and general. The anonymize option is not a Google Analytics option (not to be confused with the very similar GA-official option anonymizeIp), thus doesn't fit in with any of the other sections.

Upon further contemplation, the name is kind of vague. Thus I will be renaming the section to pluginSpecific and adding a comment to clarify what the section is for. 🔥

@wardpeet wardpeet changed the title feat(gatsby-plugin-google-analytics) Added more options. feat(gatsby-plugin-google-analytics): enable more options. Jul 9, 2019
@tinder-kyleboss tinder-kyleboss requested a review from a team as a code owner July 16, 2019 18:31
@tinder-kyleboss
Copy link
Contributor Author

tinder-kyleboss commented Jul 16, 2019 •
edited
Loading

Quick update @wardpeet!

I decided too get rid of the aLaCarte section of the knownOptions object. Looking at it again, I realized it has no functional use being there. Thanks for catching this one. ⚾️

Feel free to let me know if you see anything else! 🙏

@tinder-kyleboss
Copy link
Contributor Author

tinder-kyleboss commented Jul 18, 2019 •
edited
Loading

I am not really sure what the ENOAUDIT message is. 😅 Is this something I broke and can fix on my end?

@tinder-kyleboss tinder-kyleboss mentioned this pull request Jul 18, 2019
Closed
Merge remote-tracking branch 'upstream/master' into topics/add-genera…
22e2eaa 
…l-options-to-gatsby-plugin-google-analytics
wardpeet
wardpeet reviewed Jul 23, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet 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, thank you! 😄

wardpeet
wardpeet approved these changes Jul 30, 2019
View reviewed changes
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Oh i never merged this one 🤦‍♂. Sorry about that, this looks great! 👍 thanks for taking the time!

@wardpeet wardpeet merged commit b3ed9be into gatsbyjs:master Jul 30, 2019
@gatsbot
Copy link

gatsbot bot commented Jul 30, 2019

Holy buckets, @tinder-kyleboss — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@wardpeet
Copy link
Contributor

Successfully published:

  • gatsby-plugin-google-analytics@2.1.5

@flegall
Copy link
Contributor

flegall commented Jul 30, 2019

Hi, I don't know if it's relevant to this PR but after bumping the google analytics plugin last night, the google analytics ssr output is broken :
Screenshot 2019-07-30 at 14 35 36

It didn't close the if () { bracket.
It doesn't impact the navigation and doesn't occur when running in develop mode.

You can reproduce it on this branch : https://github.com/flegall/florent-legall-dot-com/tree/google_analytics_ssr_issue :
yarn && yarn start:prod

tinder-kyleboss added a commit to tinder-kyleboss/gatsby that referenced this pull request Jul 30, 2019
@tinder-kyleboss
feat(gatsby-plugin-google-analytics): enable more options. (gatsbyjs#…
29154f9 
…15280)

gatsbyjs@b3ed9be
had a syntax error where, after transpiling the javascript, users
are given a syntax error.

There was an if-statement which ends up not being closed.
This commit fixes that.
@tinder-kyleboss tinder-kyleboss mentioned this pull request Jul 30, 2019
Merged
@tinder-kyleboss
Copy link
Contributor Author

tinder-kyleboss commented Jul 30, 2019 •
edited
Loading

Darn! Thank you so much for catching this @flegall. 🙏 It totally is relevant and caused by this PR.

I just opened up the PR #16223 to fix this issue.

@tinder-kyleboss tinder-kyleboss mentioned this pull request Jul 30, 2019
Closed
@flegall
Copy link
Contributor

flegall commented Jul 30, 2019 •
edited
Loading

Nice ! Most of the reporting work was actually done by renovate & cypress !

I'm receiving PRs for my website from renovate (that get merged automatically if they pass).
Latest PR to upgrade gatsby failed due to an error in the few cypress e2e tests I've written for my blog.
Cypress failed because it caught the error in the JS code caused by the missing "}" !

It could have gone unnoticed though, as the whole site was working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wardpeet wardpeet wardpeet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tinder-kyleboss @wardpeet @flegall