Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't polyfill globalThis for Cloudflare preset #192

Open
IgorMinar opened this issue May 21, 2024 · 3 comments
Open

Don't polyfill globalThis for Cloudflare preset #192

IgorMinar opened this issue May 21, 2024 · 3 comments
Labels
cloudflare enhancement New feature or request

Comments

@IgorMinar
Copy link
Collaborator

IgorMinar commented May 21, 2024

Environment

unenv @ main (c4be2cb)

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:

process: "unenv/runtime/polyfill/process",

This process polyfill then pulls in globalThis polyfiil:

import _global from "./global-this";

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

// node_modules/.pnpm/[email protected]/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/[email protected]/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

@pi0 pi0 added enhancement New feature or request cloudflare labels May 21, 2024
@IgorMinar
Copy link
Collaborator Author

@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
Copy link
Member

pi0 commented May 21, 2024

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
Copy link
Member

pi0 commented Jun 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloudflare enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants