fix(cloudflare): make sure not fallback to `globalThis` for env #183

jculvey · 2024-05-15T05:07:58Z

🔗 Linked issue

❓ Type of change

📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality like performance)
✨ New feature (a non-breaking change that adds functionality)
🧹 Chore (updates to the build process or auxiliary tools and libraries)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

📝 Checklist

Adds a polyfill for process to the Cloudflare preset which combines the node polyfill with the workerd polyfill implementation. We'd specifically like to retain the polyfilled nextTick from workerd which uses queueMicrotask, but benefit from all the other global process polyfills provided by unenv.

I have linked an issue or discussion.
I have updated the documentation accordingly.

pi0 · 2024-05-15T05:19:01Z

Can you please update the polyfill to explictly override nextTick impl?

(env implementation is more generic in unenv. we can explicitly add any more override in the future that needed)

Also would it make sense to have a generic polyfills/cloudflare file that adds more of future polyfills via one entry?

jculvey · 2024-05-15T05:25:49Z

Can you please update the polyfill to explictly override nextTick impl?

(env implementation is more generic in unenv. we can explicitly add any more override in the future that needed)

Also would it make sense to have a generic polyfills/cloudflare file that adds more of future polyfills via one entry?

Great suggestions, updated!

Thanks for the speedy response time 😄.

pi0 · 2024-05-15T07:35:04Z

src/runtime/polyfill/cloudflare.ts

+import { nextTick } from "node:process";
+import { process as _process } from "../node/process/_process";
+
+Object.assign(_process, { nextTick });


Another suggestion: Considering this effectively uses a wrapper around global queueMicrotask in worked, would it make sense that we avoid node: imports or do you see that in the future workerd might do it differently?

And one more! We could also prefer (if available) queueMicrotask in main process polyfill with an early return. This makes the fix generic for all compatible runtimes.

And one more! We could also prefer (if available) queueMicrotask in main process polyfill with an early return. This makes the fix generic for all compatible runtimes.

That polyfill relies on setTimeout to schedule the work which is not the same as queueMicrotask — microtasks take precedence over tasks, so the current polyfill will run the callbacks in incorrect order.

Is there any reason for the nextTick polyfill not to use queueMicrotask? It seems to be widely supported API: https://developer.mozilla.org/en-US/docs/Web/API/queueMicrotask

would it make sense that we avoid node: imports or do you see that in the future workerd might do it differently?

We do expect the workerd runtime to get a few more (process) APIs polyfilled over time, but more importantly this PR is an attempt to create a hybrid polyfill at a symbol level, similarly how #181 proposes it for the module level. We want to prove that this is possible, as that would then inform our overall node compat strategy.

No reason main polyfill not using queueMicrotask it was simply remaining from the legacy polyfills I used.

We can use a condition in there to use queueMicrotask when available and eventually drop fallback if most of the runtimes support it.

We want to prove that this is possible, as that would then inform our overall node compat strategy.

If it is helpful for your testing strategy, looks good to me this way 👍🏼

From #181 (comment) I was under the impression that you would want to go with process.getBuiltinModule API and that this PR particularly can be natively improved in main unenv's nextTick polyfill.

@pi0

I ended up importing node:process this way because I couldn't find another way to grab the native workerd global process in a reliable way. Is there a better way of doing it?

These all didn't seem to work:

import _global from "./global-this"; console.log(_global.process); // undefined console.log(process); //undefined console.log(globalThis.process); // undefined console.log(global.process); // undefined

@jculvey the issue here is that workerd doesn't expose the process object on global at all (unless you use the underutilized nodeJsCompatModule, which we want to avoid because we are thinking of removing it).

globalThis.process and node:process seem to have the same shape however, with the exception of node:process#default which exists to support default import.

Right, my mistake. Thanks!

IgorMinar · 2024-05-15T15:21:33Z

src/runtime/polyfill/cloudflare.ts

+import { nextTick } from "node:process";
+import { process as _process } from "../node/process/_process";
+
+Object.assign(_process, { nextTick });


And one more! We could also prefer (if available) queueMicrotask in main process polyfill with an early return. This makes the fix generic for all compatible runtimes.

That polyfill relies on setTimeout to schedule the work which is not the same as queueMicrotask — microtasks take precedence over tasks, so the current polyfill will run the callbacks in incorrect order.

Is there any reason for the nextTick polyfill not to use queueMicrotask? It seems to be widely supported API: https://developer.mozilla.org/en-US/docs/Web/API/queueMicrotask

would it make sense that we avoid node: imports or do you see that in the future workerd might do it differently?

We do expect the workerd runtime to get a few more (process) APIs polyfilled over time, but more importantly this PR is an attempt to create a hybrid polyfill at a symbol level, similarly how #181 proposes it for the module level. We want to prove that this is possible, as that would then inform our overall node compat strategy.

IgorMinar · 2024-05-15T15:25:30Z

src/runtime/polyfill/cloudflare.ts

@@ -0,0 +1,4 @@
+import { nextTick } from "node:process";
+import { process as _process } from "../node/process/_process";


can you add a comment here about the issue with importing ../node/process/import directly? it might be tempting for someone to refactor this in the future and run into the issues with messed up bundling we saw and can't explain.

IgorMinar · 2024-05-15T15:28:55Z

src/runtime/polyfill/cloudflare.ts

@@ -0,0 +1,4 @@
+import { nextTick } from "node:process";


didn't you want to import the whole module and spread it? or would that introduce some additional properties on process that should not be on the global? (e.g. default`).

ah.. I see that @pi0 requested this change. I think we could keep as is then, and we can reconsider once there are more keys we'd like to use from the native layer.

However, I'm not quite sure about the process.env implementation in unenv:

unenv/src/runtime/node/process/_process.ts

Lines 160 to 187 in bf79a17

const _envShim = Object.create(null);

const _processEnv = globalThis.process?.env;

const _getEnv = (useShim: boolean) =>

_processEnv || globalThis.__env__ || (useShim ? _envShim : globalThis);

process.env = new Proxy(_envShim, {

get(_, prop) {

const env = _getEnv();

return env[prop] ?? _envShim[prop];

},

has(_, prop) {

const env = _getEnv();

return prop in env || prop in _envShim;

},

set(_, prop, value) {

const env = _getEnv(true);

env[prop] = value;

return true;

},

deleteProperty(_, prop) {

const env = _getEnv(true);

delete env[prop];

},

ownKeys() {

const env = _getEnv();

return Object.keys(env);

},

});

It comes across to me as very convoluted and potentially incorrect as reads and write target different underlying storage. @pi0 can you shed some light on why the env polyfill works this way? it might be the best to add a comment into that code to explain the reasoning.

This allows Nitro users to access variables using process.env[key] (this is not normally possible via cloudflare edge). __env__ is injected in nitro's cf entrypoint.

I would be happy to explain more later if you wanted and see how to improve it :)

I see. I think I understand how nitro usesglobalThis.__env__ and I don't mind that part, but what I find problematic is the fallback on globalThis which is used as the fallback here:

https://github.com/unjs/unenv/blob/bf79a17667cc8c8f0794800e1a50dea7e4c967cc/src/runtime/node/process/_process.ts#L163C41-L163C72

If I'm reading the code right, then n an environment like workerd, we'd end up reading globalThis.foo when someone tries to access globalThis.process.env.foo - this is not desirable for us.

I'm also puzzled about these two lines in get and has:

unenv/src/runtime/node/process/_process.ts

Line 168 in bf79a17

return env[prop] ?? _envShim[prop];

unenv/src/runtime/node/process/_process.ts

Line 172 in bf79a17

return prop in env || prop in _envShim;

Since these two lines prefer env over _envShim, this means that any assignments or deletions performed on process.env will not be observable/readable if the key is present on the original env. This seems like a bug to me.

If I'm reading the code right, then in an environment like workerd, we'd end up reading globalThis.foo when someone tries to access globalThis.process.env.foo - this is not desirable for us.

Indeed and it was because of cloudlare service-worker format support actually. As far as I remember it was a requirement for the cloudflare (legacy) sw-worker format that bindings (env vars) were accessible via globalThis.*. (docs)

I am up to avoid this and probably workaround in nitro legacy worker preset but it needs a major version of unenv or at least waiting for a minor version of Nitro to allow this.

A quick solution would that we can set globalthis.__env__ to an empty object in polyfill/cloudflare.ts since it targets new formats, basically disables the legacy behavior without breaking changes.

Indeed and it was because of cloudlare service-worker format support actually. As far as I remember it was a requirement for the cloudflare (legacy) sw-worker format that bindings (env vars) were accessible via globalThis.*. (docs)

fascinating! looks like we've made a full circle here. I actually thought of the idea to initialize globalThis.__env__ but wanted to understand the motivation behind the the fallback on globalThis. Give that it exists to support the legacy service worker format of cloudflare workers, I am fairly sure that it's safe to stop supporting that in unenv and nobody will notice. If we work around this just in the cloudflare preset, the same set of people (if they do exist afterall) would be affected, so I'd rather take the risk and clean this up for everyone.

btw do you have any thoughts on the bug I pointed out in the last paragraph of my previous comment?

nitro has not yet adopted cloudflare preset of unenv so it won't affect legacy users 🤞🏼

regarding issue seems valid (and honestly this polyfill is hacky anyway), i guess as long as we have __env__ it will use consistent source right?

BTW there is almost same impl in std-env (standalone util). We can probably test different scenarios better there using vitest and backport to unenv. (I plan to merge at some point in the future)

how about this approach: #186

pi0 · 2024-05-15T19:34:04Z

Created #184 for nextTick ~> queueMicrotask as general improvement.

On this PR, I would suggest to introduce runtime/polyfill/cloudflare.ts mainly to set __env__ shim override (to solve glboalThis concern of process.env #183 (comment)) and wait for process.getBuiltinModule to be landing before trying the mixed method.

Using node:process import idea, is probably not going to work well in practice because either the unenv cloudflare preset should configure bundler to alias node:process (to unenv) or use external one. If it is aliased to unenv, it will make a recursive situation and if it is external unenv is not effective.

IgorMinar · 2024-05-15T21:07:00Z

On this PR, I would suggest to introduce runtime/polyfill/cloudflare.ts mainly to set env shim override (to solve glboalThis concern of process.env #183 (comment)) and wait for #182 to be landing before trying the mixed method.

I addressed this in #183 (comment)

Using node:process import idea, is probably not going to work well in practice because either the unenv cloudflare preset should configure bundler to alias node:process (to unenv) or use external one. If it is aliased to unenv, it will make a recursive situation and if it is external unenv is not effective.

if we resolve the process.env issue as suggested above, we might not need to import node:process at all, but if we do, we would only use it for APIs that are natively implemented in workerd, which will remain exposed once we implement the hybrid polyfills as described in #181

so I think the last remaining thing here is to resolve the two issues I broght up with the current process.env polyfill (1/ removal of globalThis fallback 2/ swapping the order in read operations).

jculvey · 2024-05-16T14:55:17Z

Update: I've changed this PR so that the polyfill simply sets globalThis.__env__ to empty object. I'll wait to proceed until we've hashed out the discussion in #186.

src/runtime/polyfill/cloudflare.ts

jculvey force-pushed the cf-process branch from e87c796 to 9e56df2 Compare May 15, 2024 05:24

pi0 reviewed May 15, 2024

View reviewed changes

IgorMinar reviewed May 15, 2024

View reviewed changes

pi0 mentioned this pull request May 15, 2024

feat(node:process): prefer queueMicrotask for process.nextTick polyfill #184

Merged

IgorMinar mentioned this pull request May 15, 2024

fix(node): rewrite the process.env polyfill #186

Closed

8 tasks

jculvey added 3 commits May 16, 2024 09:44

feat(cloudflare): add global process polyfill to cloudflare preset

12b07a6

Addressing PR feedback

3468d3a

Trim down cloudflare polyfill

2eff838

jculvey force-pushed the cf-process branch from 9e56df2 to 2eff838 Compare May 16, 2024 14:53

pi0 reviewed May 16, 2024

View reviewed changes

src/runtime/polyfill/cloudflare.ts Show resolved Hide resolved

pi0 reviewed May 21, 2024

View reviewed changes

src/runtime/polyfill/cloudflare.ts Show resolved Hide resolved

Apply suggestions from code review

47c64f8

pi0 changed the title ~~feat(cloudflare): add global process polyfill to cloudflare preset~~ fix(cloudflare): make sure not fallback to globalThis for env May 21, 2024

pi0 approved these changes May 21, 2024

View reviewed changes

pi0 merged commit 9b1c18c into unjs:main May 21, 2024
2 checks passed

jculvey mentioned this pull request May 22, 2024

chore(cloudflare): revert #183 #195

Merged

8 tasks

pi0 pushed a commit that referenced this pull request May 22, 2024

chore(cloudflare): revert #183 (#195)

3676bf1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudflare): make sure not fallback to `globalThis` for env #183

fix(cloudflare): make sure not fallback to `globalThis` for env #183

jculvey commented May 15, 2024

pi0 commented May 15, 2024

jculvey commented May 15, 2024

pi0 May 15, 2024

pi0 May 15, 2024

IgorMinar May 15, 2024

pi0 May 15, 2024

jculvey May 15, 2024 •

edited

Loading

IgorMinar May 15, 2024

IgorMinar May 15, 2024

jculvey May 15, 2024

IgorMinar May 15, 2024

IgorMinar May 15, 2024

IgorMinar May 15, 2024

IgorMinar May 15, 2024

pi0 May 15, 2024 •

edited

Loading

IgorMinar May 15, 2024

pi0 May 15, 2024 •

edited

Loading

IgorMinar May 15, 2024

pi0 May 15, 2024

pi0 May 15, 2024 •

edited

Loading

pi0 May 15, 2024

IgorMinar May 15, 2024 •

edited

Loading

pi0 commented May 15, 2024 •

edited

Loading

IgorMinar commented May 15, 2024

jculvey commented May 16, 2024 •

edited

Loading

		@@ -0,0 +1,4 @@
		import { nextTick } from "node:process";
		import { process as _process } from "../node/process/_process";

	const _envShim = Object.create(null);
	const _processEnv = globalThis.process?.env;
	const _getEnv = (useShim: boolean) =>
	_processEnv \|\| globalThis.__env__ \|\| (useShim ? _envShim : globalThis);

	process.env = new Proxy(_envShim, {
	get(_, prop) {
	const env = _getEnv();
	return env[prop] ?? _envShim[prop];
	},
	has(_, prop) {
	const env = _getEnv();
	return prop in env \|\| prop in _envShim;
	},
	set(_, prop, value) {
	const env = _getEnv(true);
	env[prop] = value;
	return true;
	},
	deleteProperty(_, prop) {
	const env = _getEnv(true);
	delete env[prop];
	},
	ownKeys() {
	const env = _getEnv();
	return Object.keys(env);
	},
	});

fix(cloudflare): make sure not fallback to globalThis for env #183

fix(cloudflare): make sure not fallback to globalThis for env #183

Conversation

jculvey commented May 15, 2024

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

pi0 commented May 15, 2024

jculvey commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jculvey May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pi0 May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pi0 May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pi0 May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar May 15, 2024 • edited Loading

Choose a reason for hiding this comment

pi0 commented May 15, 2024 • edited Loading

IgorMinar commented May 15, 2024

jculvey commented May 16, 2024 • edited Loading

fix(cloudflare): make sure not fallback to `globalThis` for env #183

fix(cloudflare): make sure not fallback to `globalThis` for env #183

jculvey May 15, 2024 •

edited

Loading

pi0 May 15, 2024 •

edited

Loading

pi0 May 15, 2024 •

edited

Loading

pi0 May 15, 2024 •

edited

Loading

IgorMinar May 15, 2024 •

edited

Loading

pi0 commented May 15, 2024 •

edited

Loading

jculvey commented May 16, 2024 •

edited

Loading