SocialGouv / matomo-next

Matomo for Next.js applications
Apache License 2.0
152 stars 22 forks source link

feat: Make `isSearchPath` a callback parameter #74

Open jdeniau opened 2 years ago

jdeniau commented 2 years ago

Force /recherche and /search pages to be search pages seems really weird, and probably and internal application need.

This PR opens the discussion about changing this behavior.

I just created a isSearchPath callback parameter that keeps your previous behaviour to avoid BC break, but I think that the proper way to do this is to drop a major tag with an empty implementation (path: string) => false, or even to have a searchPath parameters :

searchPaths: [
  '/search.html', // for exact match
  /\/recherche/ // for a pattern matching that can replace your `startWith` call
]
revolunet commented 2 years ago

Thanks for your help on this !

What about releasing a non breaking change with an additional, optional searchPaths parameter that defaults to these values ?

jdeniau commented 2 years ago

I tried to do so, but I found another issue : the query param q is extracted from the route, and should be a parameter too.

So I think the callback may be a better option and could return the search query, but the implementation might be complex :

const defaultIsSearchPath = (path: string): string | undefined => {
  const matches = path.match(/\?(.*)/);
  if (!matches) {
    return;
  }

  const searchParams = new URLSearchParams(matches )[1]
  if (startsWith(path, "/recherche") || startsWith(path, "/search")) {
    return searchParams.get('q');
  }
}

Or if we extract the query parameters for the callback

const defaultIsSearchPath = (path: string, searchParams: URLSearchParams): string | undefined => {
  if (startsWith(path, "/recherche") || startsWith(path, "/search")) {
    return searchParams.get('q');
  }
}

But I do not really like either implementations. What do you think ?

razzeee commented 5 months ago

This would be helpful, as our search page is under /apps/search