fix: check property descriptors for configurability before polyfill

[justjavac]

Before installing polyfills, other libraries may have already Polyfilled, and these libraries may have set incorrect configurable or/and writable, which can cause the failure of executing Object.defineProperty. For example, the vscode extension Console Ninja can prevent the startup of yarn dev with error:

error when starting dev server:
TypeError: Cannot redefine property: crypto
    at Function.defineProperty (<anonymous>)
    at installPolyfills (file:///Users/justjavac/work/op/wms-web/node_modules/@sveltejs/kit/src/exports/node/polyfills.js:35:10)
    at dev (file:///Users/justjavac/work/op/wms-web/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:29:3)
    at configureServer (file:///Users/justjavac/work/op/wms-web/node_modules/@sveltejs/kit/src/exports/vite/index.js:605:17)
    at _createServer (file:///Users/justjavac/work/op/wms-web/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:63461:30)
    at async CAC.<anonymous> (file:///Users/justjavac/work/op/wms-web/node_modules/vite/dist/node/cli.js:733:24)

---

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 3038c7d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ghostdevv]

I think this will close #10252

[Conduitry]

I'm still really confused how a VS Code extension is injecting itself into the Node runtime. That sounds bad, especially if it's written in such a way that breaks other tools trying to install their own polyfills later.

Is there some way we could be more forceful in trying to overwrite these polyfills? Mark the existing values as configurable, maybe?

If we can't do that for some reason, I'd want us to be throwing a more helpful error here, rather than just accepting the pre-existing polyfilly. We have no idea how old that's going to be, or what features it's going to be missing, or even whether it's actually a polyfill of the API we want. If we just continuing on our way here - even with a warning - this is much more likely to result in bizarre behavior for someone that is going to be almost impossible to debug for everyone else because "installed this fucked-up editor extension" isn't going to be one of the reproduction steps.

[justjavac]

@Conduitry https://marketplace.visualstudio.com/items?itemName=WallabyJs.console-ninja#how-does-it-work

To integrate with supported tools seamlessly, Console Ninja patches your locally installed node modules.

[justjavac]

I checked some frameworks supported by Console Ninja like next/astro, they use webapi polyfill and it works fine:

• https://github.com/withastro/astro/blob/40029e1af400354ae726526a975573e37f5ac187/packages/webapi/src/polyfill.ts#L257-L258

  // skip WebAPIs that are built-in
  if (Object.hasOwnProperty.call(target, name)) continue

• https://github.com/vercel/next.js/blob/canary/packages/next/src/server/node-polyfill-crypto.ts#L3

  if (!global.crypto) {
     ...

I think this should be a very common usage pattern: "polyfill if not exist".

[dummdidumm]

@Conduitry correct me if I'm wrong but I believe we did not do this because for example undici is built-in to Node 18 but which version you get depends on what Node version you have installed - and some versions of undici are prone to memory leaks etc. So we want to ensure we're installing the latest version there. As for other modules such as crypto that's different as we're in fact just using the Node built-in, so I would be fine with a "don't patch if it exists" approach. @Conduitry / @benmccann thoughts?

[benmccann]

I did some sleuthing of old commits and found the reason this isn't safe: #8991 (comment). I think we could change this in SvelteKit 2.0 by dropping versions of Node older than 18.11, but I think we should probably close this PR until then - especially since the issue this was attempting to fix (#10252) has already been closed and can be addressed by upgrading to Console Ninja > v0.0.167

[benmccann]

I filed an issue to track this for Kit 2.0 so that we won't forget: #10527

[Conduitry]

@dummdidumm Sorry, I had missed your message before. For undici, yeah, there are memory leak concerns - but there are also concerns where certain versions of it just didn't support parsing multipart form data. I suspect there are versions of Node out there that we claim to support in Kit but which come with a version of undici that doesn't support multipart form data, which would break a lot of people's apps.

I'm in general really wary about defaulting to using the versions of anything that is already there, even for non-undici stuff. It would at least very likely need to be marked as a breaking change, and I don't want to have to worry about the implications of doing that.

The original version of this PR (6af78f9) actually feels a lot safer to me than what it has now. If a property on globalThis is not configurable, there's really nothing we can do, and we can either abort (which is what we're doing now, implicitly) or we can carry on and hope for the best (might hide some errors so they don't emerge until later, but probably not). If a property on globalThis is set but is still configurable, we definitely should continue to overwrite it with what we know we want to use.

As I write this, I see that @benmccann has just now closed the PR, which does, all around, seem the safest to me. If we don't have to work around another project setting this as a non-configurable global, great.