unjs / unenv

🕊️ Convert javaScript code to be runtime agnostic
MIT License
342 stars 18 forks source link

Don't polyfill globalThis for Cloudflare preset #192

Open IgorMinar opened 1 month ago

IgorMinar commented 1 month ago

Environment

unenv @ main (c4be2cb505b159a21a42920da925aff6e077f831)

Reproduction

Simple worker like:

export default {
    async fetch(request, env, ctx) {
        return new Response('Hello World!');
    },
};

Is due to inject for global process in Nodeless preset which the Cloudflare preset depends on:

https://github.com/unjs/unenv/blob/c4be2cb505b159a21a42920da925aff6e077f831/src/presets/nodeless.ts#L52

This process polyfill then pulls in globalThis polyfiil: https://github.com/unjs/unenv/blob/c4be2cb505b159a21a42920da925aff6e077f831/src/runtime/polyfill/process.ts#L2

which then due to suboptimal code generation by ESBuild's inject, results in unnecessary globalThis polyfill in Cloudflare Workers apps:

// node_modules/.pnpm/unenv@1.9.0/node_modules/unenv/runtime/polyfill/global-this.mjs
function getGlobal() {
  if (typeof globalThis !== "undefined") {
    return globalThis;
  }
  if (typeof self !== "undefined") {
    return self;
  }
  if (typeof window !== "undefined") {
    return window;
  }
  if (typeof globalThis !== "undefined") {
    return globalThis;
  }
  return {};
}
var global_this_default = getGlobal();

// node_modules/.pnpm/unenv@1.9.0/node_modules/unenv/runtime/polyfill/process.mjs
var process_default = global_this_default.process;

// src/index.js
var src_default = {
  async fetch(request, env, ctx) {
    return new Response("Hello World!");
  }
};
export {
  src_default as default
};
//# sourceMappingURL=index.js.map

Describe the bug

Ideally, we'd use straight globalThis directly in the process polyfill, and in environments which don't reliably provide globalThis we inject the polyfill via the preset configuration.

Additional context

No response

Logs

No response

IgorMinar commented 1 month ago

@pi0 I've looked up the compat table just to realize that globalThis has been around for ages (since Node v12 for example) and also all browsers support it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#browser_compatibility

Could we simply drop the globalThis polyfill since it's extremely unlikely that it's having any effect and it only adds bloat? As far as I can tell this would be a non-breaking change, and could be done as a part of the 1.9.x release train. Thoughts?

pi0 commented 1 month ago

I think mainly service worker and web workers lack globalThis instead of self that need support (not sure if it is changed in major of browsers but ~2-3 years ago that added polyfill it was a hard requirement -- unenv also make it possible to run on sw). This article also worth to read if interested.


Re cloudflare and modern runtime presets that we are sure have it, we might try to adopt an aliasing strategy. Make a generic "#unenv/global-this": "polyfill/global-this" and map it to "#unenv/global-this-native" for cloudflare preset wdyt?

pi0 commented 2 weeks ago

@IgorMinar Can this be closed? (haven't went though $cloudflare variants)