fix: make injectQuery idempotent

[markdalgleish]

Description

This fixes the following issue with the injectQuery util:

injectQuery('/foo?import', 'import');
// -> '/foo?import&import'

I ran into this because we're experimenting with using Vite for Remix. Remix injects a script tag in the HTML from the server to load all the JS modules for the current route before running the app, e.g.:

<script type="module" async="">
import * as route0 from "/@fs/path/to/app/root.tsx";
import * as route1 from "/@fs/path/to/app/routes/_index.tsx";
window.__remixRouteModules = {"root":route0,"routes/_index":route1};

import("/@fs/path/to/app/entry.client.tsx");
</script>

However, this fails in Vite when using MDX routes since the Vite dev server responds with the raw MDX content.

<script type="module" async="">
import * as route0 from "/@fs/path/to/app/root.tsx";
import * as route1 from "/@fs/path/to/app/routes/mdx/route.mdx"; // <-- THIS IS AN ERROR 😭
window.__remixRouteModules = {"root":route0,"routes/mdx":route1};

import("/@fs/path/to/app/entry.client.tsx");
</script>

To fix this, I appended ?import to the route module URLs in the Remix manifest in dev mode so that the Vite dev server responds with the compiled JS. This ensures that module URLs in the Remix manifest are valid JS modules, even when using other file formats. This means the injected script tag is now correct:

<script type="module" async="">
import * as route0 from "/@fs/path/to/app/root.tsx";
import * as route1 from "/@fs/path/to/app/routes/mdx/route.mdx?import"; // <-- THIS WORKS! 🎉
window.__remixRouteModules = {"root":route0,"routes/mdx":route1};

import("/@fs/path/to/app/entry.client.tsx");
</script>

However this causes issues when these URLs are imported within Remix library code because Vite transforms the import statement to use injectQuery which turns the query string into ?import&import.

To fix this, I've updated injectQuery to detect whether the queryToInject argument is already present in the search params and that it has a blank value, and if so, first delete any values for queryToInject from the search params to avoid adding it again.

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[markdalgleish]

@bluwy Thanks for the feedback! I've made the changes you've asked for, let me know what you think.

[bluwy]

Forgot to followup on this! Thanks for making the changes, I have one small nit below. Before merging this, I'd like another eye from others too.

Presumably this will have small perf implications, but I think it shouldn't be a lot. Any other way to handle duplicate params would be pretty hard.

[bluwy]

Just ran a perf test and it seems like this PR makes it ~60% slower, which is not quite ideal 🤔

I implemented an alternative approach which failed one test case "path with injected query already present multiple times", but is still 30% slower.

Snippet you can paste into perf.link

const postfixRE = /[?#].*$/s
 function cleanUrl(url) {
  return url.replace(postfixRE, '')
}

const replacePercentageRE = /%/g
function injectQuery(url, queryToInject) {
  const resolvedUrl = new URL(
    url.replace(replacePercentageRE, '%25'),
    'relative:///',
  )
  let { search, hash } = resolvedUrl
  let pathname = cleanUrl(url)
  const queryKey = queryToInject.split('=')[0]
  if (search.includes(`?${queryKey}`) || search.includes(`&${queryKey}`)) {
    search = search.replace(
      new RegExp(`(\\?|&)${queryKey}=?.*?(&|$)`, 'g'),
      (_, m1, m2) => (m2 ? m1 : ''),
    )
  }
  return `${pathname}?${queryToInject}${search ? `&` + search.slice(1) : ''}${
    hash ?? ''
  }`
}

for (const [key, value] of Object.entries(data)) {
  for (let i = 0; i < 100; i++) {
    injectQuery(key, value)
  }
}

Maybe it's easier to fix the specific case you're encountering for now, since it's not often we get this issue. Something like:

if (search.startsWith(`?${queryToInject}`)) return url

Should be enough, and we can keep and skip the tests (that now fails) for future reference. There's still ~8% perf impact, but I think that's small enough.

[patak-dev]

I think it would be ok to accept the performance hit to injectQuery, but IIUC, last time we got confronted with this issue, we decided to avoid pushing for the use of ?import in downstream projects (as it may be removed at one point in the future). And as a temporal patch, we got the extension added to our harcoded list of known JS sources here:

• feat: Add support for imba in html scripts #10679

Maybe mdx should be added too for now?

[bluwy]

Hmm if that works I'm also ok with that. I thought there would be more to the MDX processing done, but so far I've only seen MDX always being treated as JS, so it should be fine.

[sapphi-red]

I think we can add mdx to the known types.

I appended ?import to the route module URLs in the Remix manifest in dev mode

Is this by hand or by a transform hook in a plugin? Would calling viteServer.transformIndexHtml or viteServer.transformRequest before/after injecting the script tag work?

[markdalgleish]

I'm just coming back to this now that we've merged the unstable Vite support PR into Remix. I'm not sure what changed but I'm finding that the ?import&import query string is not causing issues for us anymore. I'm closing this since nobody else was asking for this. Thanks for the help!