deps(browser): remove modern-node-polyfills package

[Marcel-G]

resolves: #3299

The browser runner indiscriminately applies pollyfills for node builtin modules. This PR removes completely removes the dependence on modern-node-polyfills.

[sheremet-va]

We don't rely on util in #3685 anymore, so we can also remove polyfills now with modern-node-polyfills package

[userquin]

The name of this plugin now doesn't seem appropriate, any naming ideas?

vitest:browser:tests

[sheremet-va]

As far as I can tell util is still included by @sinonjs/fake-timers:

vitest/packages/vitest/src/integrations/mock/timers.ts

Line 38 in c8c1b4a

this._fakeTimers = withGlobal(global)

1. util is included dynamically if running inside Node.js
2. We bundle @sinonjs/fake-timers

[sheremet-va]

Heres the error in the browser if we don't include util pollyfil

__vite-browser-external:util:3 Uncaught (in promise) Error: Module "util" has been externalized for browser compatibility. Cannot access "util.default" in client code.  See http://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.
    at Object.get (__vite-browser-external:util:3:11)
    at withGlobal (vendor-vi.f4f5ef45.js:1370:47)
    at vendor-vi.f4f5ef45.js:3017:32
    at vendor-vi.f4f5ef45.js:3023:3

Probably because we have process mocked, which we also shouldn't do, but it should be addressed in a separate issue.

[Marcel-G]

Probably because we have process mocked, which we also shouldn't do, but it should be addressed in a separate issue.

Something like this #3701?

[sheremet-va]

Probably because we have process mocked, which we also shouldn't do, but it should be addressed in a separate issue.

Something like this #3701?

Yes

[sheremet-va]

Heres the error in the browser if we don't include util pollyfil

Why are tests passing then 🤔

[Marcel-G]

OK, the tests will fail until #3701 is in

[sheremet-va]

normalizeId is not used anymore

[netlify]

✅ Deploy Preview for fastidious-cascaron-4ded94 ready!

Name	Link
🔨 Latest commit	0f333f8
🔍 Latest deploy log	https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/650d90ed8063f40008426c62
😎 Deploy Preview	https://deploy-preview-3648--fastidious-cascaron-4ded94.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[Marcel-G]

normalizeId is not used anymore

Very true! Removed ✅

[Marcel-G]

@sheremet-va are the tests generally a bit flakey? It seems to be a different spec failing each time

[sheremet-va]

@sheremet-va are the tests generally a bit flakey? It seems to be a different spec failing each time

Some tests are flaky. Your PR is open until we have a browser test in ecosystem-ci

[Marcel-G]

Some tests are flaky. Your PR is open until we have a browser test in ecosystem-ci

@sheremet-va what is the status of that? is it something I can help with?

[sheremet-va]

Some tests are flaky. Your PR is open until we have a browser test in ecosystem-ci

@sheremet-va what is the status of that? is it something I can help with?

Sorry for keeping you waiting! I am working on a small test suite for ecosystem-ci now, we can probably merge this PR today or next week.

[AriPerkkio]

How should the util be polyfilled now? It's still used by @sinonjs/fake-timers. v1-beta fails with error SyntaxError: The requested module '/node_modules/.pnpm/util@0.12.5/node_modules/util/util.js?v=c8f99d41' does not provide an export named '__vi_inject__'. And when adding "vitest > util" to optimizeDeps.include, it fails with:

ReferenceError: process is not defined
 ❯ node_modules/.pnpm/util@0.12.5/node_modules/util/util.js ../../../../node_modules/.vite/deps/vitest___util.js:1308:5
 ❯ __require ../../../../node_modules/.vite/deps/chunk-UXIASGQL.js:9:50
 ❯ ../../../../node_modules/.vite/deps/vitest___util.js:1799:25

[sheremet-va]

How should the util be polyfilled now?

Vitest doesn't use it and as far as I know @sinonjs/fake-timers checks it at runtime, so this shouldn't be a problem? If it is, maybe we can patch it and inline the package?

And when adding "vitest > util" to optimizeDeps.include, it fails with:

The error looks correct to me since we don't define a global process variable since it doesn't exist in the browser.

[AriPerkkio]

Vitest doesn't use it

Looks like @vitest/browser imports vitest/runners, which uses vi. Importing vi brings in inlined fake-timers which has process. The vendor-vi has import require$$2 from 'util';.

[sheremet-va]

Vitest doesn't use it

Looks like @vitest/browser imports vitest/runners, which uses vi. Importing vi brings in inlined fake-timers which has process. The vendor-vi has import require$$2 from 'util';.

fake-timers have a check for process. What I assume happened in your case is that polyfill is using process for some reason.

The vendor-vi has import require$$2 from 'util';.

Yeah, this is why I am saying we should just patch fake timers to remove this check.