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 pathname to analytics.page() #47

Closed xtellurian closed 2 years ago

xtellurian commented 2 years ago

Adds the pathname to analytics.page() call

Fix #13

xtellurian commented 2 years ago

@benjaminhoffman I'm not familiar with this codebase, but I think this is a good way to get the actual path for the segment payload

benjaminhoffman commented 2 years ago

yeah im not sure the best way for this. i defer to @newhouse for official review now :)

newhouse commented 2 years ago

Will have a look as soon as I can. Looks good at first glance though.

newhouse commented 2 years ago

@xtellurian while this code looks simple enough, I'm struggling to understand the issue that it's solving, and when. Is this about canonical link issues, still?

More importantly, I wonder if this code change will actually break behavior for some users in some cases.

Some more explanation would be appreciated, and also any code samples that I can use to replicate the issue/scenario this is solving for would be great!

xtellurian commented 2 years ago

@newhouse I'm using this plugin in production. The paths I get in my segment events are all empty. My workaround is to manually call analytics.page()

I don't think this plugin correctly captures the path when analytics.page() is called.

But I can see that analytics.js theoretically adds the path to all page() calls - https://segment.com/docs/connections/sources/catalog/libraries/website/javascript/

benjaminhoffman commented 2 years ago

@xtellurian we've seen issues in the past around page calls. it's very strange and I wouldn't rule out segment itself (I worked there as an engineer and we all knew page calls were kinda shaky anyway... we used track calls instead).

see here: https://github.com/benjaminhoffman/gatsby-plugin-segment-js/issues?q=path

newhouse commented 2 years ago

@newhouse I'm using this plugin in production. The paths I get in my segment events are all empty. My workaround is to manually call analytics.page()

I don't think this plugin correctly captures the path when analytics.page() is called.

But I can see that analytics.js theoretically adds the path to all page() calls - https://segment.com/docs/connections/sources/catalog/libraries/website/javascript/

Not that the effort is not appreciated, but I think that for now, unless you can provide a simple way to reproduce this that I can use to investigate and understand some more, it's probably not a good idea to merge this in.

And unless the plugin is definitely not allowing segment defaults to do their thing for 100% of users of this plugin, this would be a "breaking change", not a bugfix.

I suspect there is something unique to your setup/use-case that's causing this to be an issue. If you can figure out what that is so I can replicate and investigate, let me know...

Closing for now...