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

Closed
wants to merge 1 commit into from
Closed

Add pathname to analytics.page() #47

wants to merge 1 commit into from

Conversation

xtellurian
Copy link

Adds the pathname to analytics.page() call

Fix #13

@xtellurian
Copy link
Author

@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
Copy link
Owner

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

@newhouse
Copy link
Collaborator

newhouse commented Feb 14, 2022

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

@newhouse newhouse self-requested a review February 14, 2022 18:20
@newhouse
Copy link
Collaborator

@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
Copy link
Author

@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
Copy link
Owner

@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
Copy link
Collaborator

newhouse commented Mar 3, 2022

@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...

@newhouse newhouse closed this Mar 3, 2022
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.

page() does not capture correct path if site uses canonical link tag
3 participants