(Example) Update with-segment-analytics to use segmentio/analytics-next and app layout

[lukebussey]

The with-segment-analytics example is out of date so this PR updates it to use segmentio/analytics-next with TypeScript and the app layout.

[lukebussey]

@silesky do I need to do anything different here?

[silesky]

@silesky do I need to do anything different here?

See my comment 😄

[lukebussey]

@silesky how's it look now?

[silesky]

@silesky how's it look now?

Looks good to me! I have been busy and haven't had time to pull it down; Did you run it once locally to see if it works as expected? (just checking). If so, I'm ready to give my ✔️

[lukebussey]

@silesky how's it look now?

Looks good to me! I have been busy and haven't had time to pull it down; Did you run it once locally to see if it works as expected? (just checking). If so, I'm ready to give my ✔️

Yep, looks good to me

[kylemh]

Next.js applications still exist that use the pages router. It would be a good idea to update the documentation for it rather than delete it outright. At the very least, I'd recommend updating the REAMDE to incuding Segment's own example, but there's looks a bit complicated... For example, it's not strictly necessary to use partytown or a context provider like they do.

[lukebussey]

I'd be happy to link to their example, but I do agree it is a bit complicated. Alternatively I would also be happy to add a separate simpler example for using Segment with the pages router also.

[kylemh]

I only happened upon this as I'm updating stuff for work. I feel lucky to have bumped into it while you're devving 😅

Curious... how different do you think the pages directory setup would be from what you have here? Is Analytics.load safe to call in the clear or is Segment's example usage of ContextProvider a necessity to avoid undefined references of window?

[silesky]

Segment's example usage of ContextProvider a necessity to avoid undefined references of window?

Segmenter here. We only use it in our example because it's more of a playground, so you can do stuff you'd never do in a normal application, like dynamically update the writeKey. See the little warning in our README. I would like to rename our Segment example to something like "next-12-playground" to make it clear it's a bit old and not a clean example.

I'm excited about this example and intend to link to it in the Segment docs/analytics readme as the source of truth for a clean Next.js 13 implementation.

PS
Open Q: if it is worth creating a clean standalone page router implementation, and if that should live here or elsewhere.

[kylemh]

it's totally fine to have it live here and differentiate the example by name:

with-segment
with-segment-pages-router

[lukebussey]

I just added a pages router example

[kylemh]

https://nextjs.org/docs/pages/api-reference/functions/use-router#routerevents

This might be better to do because asPath might represent the actual file path instead of a changed route. If you want to keep using asPath, I'd conditionally call page() only if router.isReady is true.

Suggested change

	const { asPath } = useRouter()
	useEffect(() => {
	analytics.page()
	}, [asPath])
	const router = useRouter()
	useEffect(() => {
	analytics.page(); // capture initial route
	router.events.on('routeChangeComplete', () => analytics.page());
	return () => {
	router.events.off('routeChangeComplete', () => analytics.page());
	}
	}, [])

[lukebussey]

How do you get the initial page view using router events? It only seems to fire after the initial page view.

[kylemh]

updated example. Alternatively, you can stick with asPath, I'd just do so conditionally after router.isReady is true.

[lukebussey]

I got it working with routeChangeComplete thanks @kylemh

[kylemh]

@leerob this is the PR I was telling you about.

[leerob]

Could you fix the ESLint errors please and we can merge? 🙏

[lukebussey]

Fixed by adding router.events to the dependency array

[lukebussey]

@leerob @silesky is there anything into else I need to do to get this merged?

[silesky]

@leerob @silesky is there anything into else I need to do to get this merged?

LGTM, you've got my 👍

[ijjk]

Allow CI Workflow Run

• approve CI run for commit: b46dfd0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

[ijjk]

Allow CI Workflow Run

• approve CI run for commit: b46dfd0

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer