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

referral query param updates #425

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

byoungdale
Copy link

This is for the query param changes for referrals mentioned in #377 .

@byoungdale
Copy link
Author

byoungdale commented Aug 19, 2023

@huumn, I tried to do the url query change in _app.js like this:

    const referralUpdate = props.me ? { r: props.me.name } : {}
    router.events.on('routeChangeStart', nprogressStart)
    router.events.on('routeChangeComplete', nprogressDone)
    router.events.on('routeChangeError', nprogressDone)

    if (!props?.apollo) return
    // HACK: 'cause there's no way to tell Next to skip SSR
    // So every page load, we modify the route in browser history
    // to point to the same page but without SSR, ie ?nodata=true
    // this nodata var will get passed to the server on back/foward and
    // 1. prevent data from reloading and 2. perserve scroll
    // (2) is not possible while intercepting nav with beforePopState
    router.replace({
      pathname: router.pathname,
      query: { ...router.query, nodata: true, ...referral }
    }, router.asPath, { ...router.options, shallow: true }).catch((e) => {
      // workaround for https://github.com/vercel/next.js/issues/37362
      if (!e.cancelled) {
        console.log(e)
        throw e
      }
    })

The issue is this didn't update the url at all.

Then I tried the below in _app.js, but it caused the page to get in some sort of re-render loop. Not 100% sure why. I will take a closer look later unless you know.

    router.events.on('routeChangeStart', nprogressStart)
    router.events.on('routeChangeComplete', nprogressDone)
    router.events.on('routeChangeError', nprogressDone)

    if (!props?.apollo) return
    // HACK: 'cause there's no way to tell Next to skip SSR
    // So every page load, we modify the route in browser history
    // to point to the same page but without SSR, ie ?nodata=true
    // this nodata var will get passed to the server on back/foward and
    // 1. prevent data from reloading and 2. perserve scroll
    // (2) is not possible while intercepting nav with beforePopState
    router.replace({
      pathname: router.pathname,
      query: { ...router.query, nodata: true }
    }, router.asPath, { ...router.options, shallow: true }).catch((e) => {
      // workaround for https://github.com/vercel/next.js/issues/37362
      if (!e.cancelled) {
        console.log(e)
        throw e
      }
    })

    if (props.me) {
      router.push({
        pathname: router.pathname,
        query: { ...router.query, r: props.me.name },
      });
    }
``

@huumn
Copy link
Member

huumn commented Aug 19, 2023

The issue is this didn't update the url at all.

router.replace takes two paths, one as the first argument and one as the second. The first argument, the one you're adding referral to, can be thought of as known to the application but hidden from the user. If you'll notice, I'm adding nodata=true but it also doesn't show in the url bar.

The second argument is what you want to modify. Here's a link to the nextjs docs for router.push (same args with same meanings as router.replace but it has more thorough param descriptions ... you'll still want to use router.replace): https://nextjs.org/docs/pages/api-reference/functions/use-router#routerpush

@byoungdale
Copy link
Author

Gotcha. Thank you. Should of looked a little closer at the code and docs. Got it working now with router.replace in _app.js. The thing is now the ?r=username query param will be added to every single page. Not only the item pages like the current referral fragment works. Is that what we want? Seems like it's not a bad idea. Users might just want to copy any url they are on and send it to someone...

pages/_app.js Show resolved Hide resolved
@huumn
Copy link
Member

huumn commented Aug 24, 2023

Thanks for this!

You did a really precise job of making these changes and I probably would've procrastinated doing this forever otherwise.

Please drop a lightning address or something when you get a change so I can tip you when I deploy this.

@huumn huumn marked this pull request as draft August 24, 2023 21:31
@ekzyis ekzyis added the feature new product features that weren't there before label Sep 7, 2023
@huumn
Copy link
Member

huumn commented Sep 18, 2023

Remaining todos:

  • provide migration path for old referral links
  • make paths as expected (discussed above)
  • meybe fix lightning login not attributing referrals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants