Would you provide concrete examples of require.toUrl(relativePath), module.uri, and document.baseURI?
Rollup has been using the same code for years, so I guess there's a reason.
Have you tried setting the baseUrl option?

Sure, no problem. I've used this code:

const customRelativeUrlMechanisms = {
  ...relativeUrlMechanisms,
  // override amd to use module.uri instead of document.baseURI
  amd: (relativePath) => {
    if (relativePath[0] !== '.') relativePath = './' + relativePath
    return getResolveUrl(
      `require.toUrl('${escapeId(relativePath)}'), (() => {
        console.log('relativePath', '${relativePath}')
        console.log('require.toUrl(relativePath)', require.toUrl('${relativePath}'))
        console.log('module.id', module.id)
        console.log('module.uri', module.uri)
        console.log('document.baseURI', document.baseURI)

        return new URL(module.uri, document.baseURI).href
      })()`,
    )
  },
  'worker-iife': (relativePath) =>
    getResolveUrl(
      `'${escapeId(partialEncodeURIPath(relativePath))}', self.location.href`,
    ),
} as const satisfies Record<string, (relativePath: string) => string>

to produce this output:

relativePath ../assets/zip-web-worker-CqnRs-V2.js
unzip.js:4681 require.toUrl(relativePath) ./../assets/zip-web-worker-CqnRs-V2.js
unzip.js:4682 module.id https://host.docker.internal:9200/assets/apps/unzip/js/unzip.js
unzip.js:4683 module.uri https://host.docker.internal:9200/assets/apps/unzip/js/unzip.js
unzip.js:4684 document.baseURI https://host.docker.internal:9200/

I'm not sure about the baseUrl - is it possible to have multiple instances of requirejs?
We have multiple apps with individual baseUrls so to speak.
If I do

requirejs.config({
    baseUrl: '/assets/apps/unzip/'
  });

that's only valid for a single app but not for all others.

No idea why rollup implemented it this way. Maybe module.uri wasn't always available.
Usually it probably just doesn't matter. I have to admit my use case is a bit of an edge case - but it's one that is supported by system and es modules.
I think it's a valid way to define "relative paths" to be relative to some base uri, but then all module systems should implement it that way.

If you insist, I can also send a PR to Rollup and we can check their feedback 😅

Thank you for the examples and the PR to Rollup. I think it makes sense. Just in case, I'd like to wait for a week for the feedback on Rollup side.

Makes sense. I understand that while being a small change in terms of LoC, the change could potentlally be delicate and I certainly don't want to break others.
If we can fix it in rollup and vite and keep the code in sync, that would of course be the best outcome.

If rollup people see a problem, I'm happy to discuss alternatives with you and them.
Until then we can certainly wait. As I wrote above it's an issue we have a workaround for, so there's absolutely no urgency to fix it... but it would be so much nicer if we could just use ?url without manual tinkering with the url (and ?worker and import other assets... )

I've tried to upstream it to rollup, but afaict now it's not applicable there, because rollup does not have the relative base feature https://vite.dev/guide/build.html#relative-base: relative paths are always relative to the baseURI not to the including module.

In vite on the other hand the code I modified/overrode is only used when the relative base feature (with base ./) is used and we have relative paths from the including module to the included module.
So I assume that's the reason the code in Rollup is what it is and why the change makes sense in vite only.

c.f. rollup/rollup#6129 (comment)

@sapphi-red Thoughts?

I think the relativePath passed to the relativeUrlMechanisms is the same value.
https://github.com/rollup/rollup/blob/55a8fd5a70820f274921edf394efbbaa620f0962/src/ast/nodes/MetaProperty.ts#L96

I think I need a reproduction to see what is happening.

In my tests it wasn't the same value but I'll try to setup a reproducer which does (not) work with vite/rollup
Then you and @lukastaegert can look at it again and maybe we find a solution that works everywhere

It's not completely trivial, so I'm not sure how soon I'll get to it...

https://github.com/dschmidt/vite-amd-relative-import-example

This shows my issue in vite for starters...

I found the difference. With Rollup, the following code is generated:

define(["require"], (function(require) {
  "use strict";
  const fooTxtUrl = new URL(require.toUrl("../assets/foo.txt"), document.baseURI).href;
  function main() {
    console.log("fooTxtUrl", fooTxtUrl);
    return "OK";
  }
  return main;
}));

On the other hand, Vite generates:

define((function() {
  "use strict";
  const fooTxtUrl = "" + new URL(require.toUrl("../assets/foo.txt"), document.baseURI).href;
  function main() {
    console.log("fooTxtUrl", fooTxtUrl);
    return "OK";
  }
  return main;
}));

(the code I used)

The problem here is that Vite is not generating ["require"] and require in the parameter. This leads to require referencing the global one instead. We need to add a plugin like this one: https://github.com/vitejs/vite/blob/2443e056941d63ea5988293ca1a1351af017e59d/packages/vite/src/node/plugins/completeSystemWrap.ts

That makes a lot of sense! I can confirm that adjusting the generated plugin.js to include the require parameter works.
Thanks a lot for digging into this!

Thanks a bunch!

Feel free to squash the commits before or when merging.

@sapphi-red can we run ecosystem-ci with this PR? iirc PWA plugin will generate some define when building a custom service worker (type classic)

/ecosystem-ci run

Open in StackBlitz

npm i https://pkg.pr.new/vite@20861

commit: 74c9da4

📝 Ran ecosystem CI on 1a8ee07: Open

✅ astro, ladle, nuxt, marko, one, histoire, quasar, sveltekit, unocss, rakkas, vike, vite-plugin-vue, vite-plugin-react, vite-plugin-pwa, vite-plugin-svelte, vite-setup-catalogue, vuepress, vitepress

All the failures are known ones. @userquin plugin-pwa passes fine with this PR 👍

Thanks everyone for the input and especially to @sapphi-red for taking the PR over the finish line :)