search

benjaminhoffman / gatsby-plugin-segment-js

Gatsby plugin for segment.com's analytic.js snippet
https://www.npmjs.com/package/gatsby-plugin-segment-js
MIT License
40 stars 28 forks source link

add a `manualLoad` option that skips calling analytics.load #34

Closed TomPridham closed 4 years ago

TomPridham commented 4 years ago

i need to wait to load analytics.js until a user accepts to be tracked on our site for GDPR, which i can't do currently. this adds an option that completely skips calling analytics.load from the plugin to allow manually calling it later.

i tested the different options({delayLoad: false, manualLoad: false}, {delayLoad: true, manualLoad: false}, {delayLoad: true, manualLoad: true}, and {delayLoad: false, manualLoad: true} and everything seemed to be working as expected. this change gives precedence to manualLoad, so if {delayLoad: true, manualLoad: true} it won't load segment at all

benjaminhoffman commented 4 years ago

@TomPridham thanks for all the attention you've been given to this plugin. i really appreciate it.

i'll carve out some time this week to review this and the other PR you have. a bit slammed at work right now.

also looking for co-owners of this repo if you're interested. you'd be allowed to merge PRs of your own or others, and possibly help answer a few of the outstanding issues.

TomPridham commented 4 years ago

no problem. it's nice finding a place i can give back to the OSS community for work :wink:

i would be interested in being a co-owner. what would be next steps for that?

benjaminhoffman commented 4 years ago

@TomPridham I invited you as a collaborator to this project. THANK YOU! I really appreciate it. this gatsby plugin is gaining in popularity and i am too swamped to keep up. once you accept, you will be able to merge PRs. once PRs are merged, you can also deploy to NPM! ... but first:

  1. what's your NPM account? i couldnt find you. in order to publish/deploy this plugin, i need to add you to that repo.

  2. please follow SEMVAR when publishing. are you familiar with this? if not, i can explain but it's relatively popular already.

  3. we have 5 PRs that need merging -- most of them are yours! haha

  4. i'd REALLY like to start integrating tests to this repo. lots of folks rely on it for collecting data and one bad deploy could accidentally fck over a bunch of people. if this is something within your bandwidth, it would be great if you could take a look.

benjaminhoffman commented 4 years ago

regarding this PR.

  1. first off, great! at least a handful of folks have asked for this feature. thank you.

  2. i think we should have better documentation about each of the use cases you mention in the comment. for example, let's create a section in the README called ### Setting use cases whereby we indicate what actually happens when you turn settings on or off. i think we are starting to get setting bloat.

  3. this is a perfect use case for writing tests. do you have the bandwidth to add them to this PR. as this plugin gains in popularity, since its the leading segment plugin for gatsby, i can imagine we will start to expand its setting scope and breakage is bound to happen.

happy to review your PRs!

TomPridham commented 4 years ago

@TomPridham I invited you as a collaborator to this project. THANK YOU! I really appreciate it. this gatsby plugin is gaining in popularity and i am too swamped to keep up. once you accept, you will be able to merge PRs. once PRs are merged, you can also deploy to NPM! ... but first:

1. what's your NPM account?  i couldnt find you.  in order to publish/deploy this plugin, i need to add you to that repo.

2. please follow [SEMVAR](https://semver.org/) when publishing.  are you familiar with this?  if not, i can explain but it's relatively popular already.

3. we have [5 PRs that need merging](https://github.com/benjaminhoffman/gatsby-plugin-segment-js/pulls) -- most of them are yours!  haha

4. i'd REALLY like to start integrating tests to this repo.  lots of folks rely on it for collecting data and one bad deploy could accidentally fck over a bunch of people.  if this is something within your bandwidth, it would be great if you could take a look.

1: it should just be tompridham. here's a link if the search isn't pulling me up for some reason: https://www.npmjs.com/~tompridham

2: yep! i'm familiar with semver. we use it for our projects at work

3: i'll have a look at the prs. it looks like i duplicated the oldest pr and it now has some merge conflicts, so i will prolly just close that in favor of mine. I haven't looked at the latest one yet, but can look next week

4: i can look at adding some tests next week. i'll also see if i can rope some of my coworkers in to write some tests as well

benjaminhoffman commented 4 years ago

fantastic. just added you to NPM. didn't realize it was case sensitive 🤷🏽‍♂️

if you guys are using this plugin in production, we should definitely get some tests rocking. im happy to contribute some too starting in early sept... my schedule opens up greatly by then.

segment offers so many options and settings. im worried about bloat. but i'm more stoked that i finally have help maintaining this plugin so feel free to tag my in all PRs for review. Thanks!