Implements globalThis.Cloudflare.compatibilityFlags API #2527

jasnell · 2024-08-13T18:36:31Z

A simple built-in API for determining if a given compat flag is set.

const { compatibilityFlags } = globalThis.Cloudflare || {};

console.log(compatibilityFlags['url_standard']);

Internal discussion wiki: (sorry external folks, you won't be able to see this) https://wiki.cfdata.org/display/~jsnell/Exposing+active+compat+flags+to+workers

src/cloudflare/workers.ts

src/workerd/api/node/node.h

src/workerd/api/tests/compat-flags-test.js

0001-Implements-cloudflare-compatibility-flags-API.patch

src/workerd/api/node/node.h

jasnell · 2024-08-15T15:12:39Z

This PR should be ready for review.

src/workerd/api/tests/compat-flags-test.js

src/workerd/api/global-scope.h

dario-piotrowicz

Looks good to me 🙂 (from just a js/ts point of view)

jasnell · 2024-08-15T17:59:08Z

This is otherwise ready to land but going to hold off until @kentonv signs off.

mhart · 2024-08-15T22:46:53Z

This looks great!

Given there's some complexity around no_* and on vs off, I think we'll need to provide clear guidance on how this should be used. ie, how we recommend people use it – and make sure it covers all the use cases it needs to.

My understanding is that 99% of the time, you're just going to want to check if a particular flag === "on", correct?

And all the flags listed here will be present: https://developers.cloudflare.com/workers/configuration/compatibility-dates/, correct?

And by default all the Flag to enable ones will be on, correct?

jasnell · 2024-08-15T23:03:00Z

Not sure I completely follow the question so hopefully this answers it by way of example:

The url_standard and url_legacy flags are mutually exclusive. If url_standard is 'on', url_legacy will be 'off'

If we have a foo and no_foo, if foo is set it will be 'on' and no_foo will be 'off'. If no_foo is set, no_foo will be 'on' and foo will be off.

And yes, tor the most part, most users will generally only be checking for === 'on'. The 'off' is largely just there to reliably differentiate between a flag that exists but is off vs. a flag that does not exist.

dario-piotrowicz · 2024-08-15T23:13:38Z

@jasnell sorry if it's a silly question... I was just wondering, would there be any benefit in also exposing the compat date?

or is it pretty unnecessary since the compat date basically just dictates the compat flags? (so those are ultimately all the ones you need to know)

jasnell · 2024-08-15T23:15:47Z

I was debating the same thing. Short answer is I don't know. I'm not sure it's going to be that useful, certainly possible tho. I wouldn't do it in this PR tho.

dario-piotrowicz · 2024-08-15T23:18:40Z

I was debating the same thing. Short answer is I don't know. I'm not sure it's going to be that useful, certainly possible tho. I wouldn't do it in this PR tho.

ok thanks, I was just curious 🙂👍

IgorMinar

I'm very supportive of this feature, but let's please not rush merging this in.

Could we please settle the following first:

why do we care about CJS support? (in other words, why not import.meta.cloudflare) https://x.com/jasnell/status/1824114538272411825
why capital C in "globalThis.Cloudflare"? As far as I can think of, it is customary to use lower-case letters in JS for variables like this one, for example globalThis.navigator or globalThis.process.
are we intentionally not exposing the compatibility_date? I'm unsure if we should, so I'm mainly wondering if the omission is intentional.
can you please document the plan for typescript support? without a design doc, it's not clear to me if this will just work or if we need to do anything special. I expect that we'll need to change RTT to support dynamic type generation that is currently in works by @andyjessop
can we ensure that these flags use an API that enable tree-shaking of the code, I don't believe that's the case now due to 'on' and 'off' values instead of using boolean type

src/workerd/api/tests/compat-flags-test.js

mhart · 2024-08-16T00:33:17Z

why capital C in "globalThis.Cloudflare"? As far as I can think of, it is customary to use lower-case letters in JS for variables like this one, for example globalThis.navigator or globalThis.process.

There's prior art here with globalThis.Deno and globalThis.Bun – lots of code checks for compatibility this way – so we have consistency there and won't stick out as the odd one out.
There's some (not much) existing code that uses lowercase globalThis.cloudflare
Unlike navigator and process, this is going to introduce a non-standard property, so it's better that it diverges from standard norms

IgorMinar · 2024-08-16T00:42:23Z

why capital C in "globalThis.Cloudflare"? As far as I can think of, it is customary to use lower-case letters in JS for variables like this one, for example globalThis.navigator or globalThis.process.

There's prior art here with globalThis.Deno and globalThis.Bun – lots of code checks for compatibility this way – so we have consistency there and won't stick out as the odd one out.

There's some (not much) existing code that uses lowercase globalThis.cloudflare

Unlike navigator and process, this is going to introduce a non-standard property, so it's better that it diverges from standard norms

These are good argument for capital C. Thanks @mhart

jasnell · 2024-08-16T00:55:31Z

... but let's please not rush merging this in.

There is no rush.

why do we care about CJS support? (in other words, why not import.meta.cloudflare)

The vast majority of modules on npm are still commonjs and will remain so for an indefinite amount of time. globalThis.Cloudflare means we can support any module, CJS or ESM. import.meta.cloudflare would mean we are intentionally choosing to ignore the majority of the very ecosystem we are working to become more compatible with.

globalThis.Cloudflare is also consistent with what other runtimes have done (e.g. globalThis.Deno, globalThis.Bun, etc)

I would also note that I've received direct feedback from ecosystem module maintainers that globalThis.Cloudflare would be their preferred approach.

are we intentionally not exposing the compatibility_date? I'm unsure if we should, so I'm mainly wondering if the omission is intentional.

#2527 (comment) ....

short answer is: I'm definitely open to adding it if there's a useful use case for it.

can you please document the plan for typescript support? without a design doc, it's not clear to me if this will just work or if we need to do anything special. I expect that we'll need to change RTT to support dynamic type generation that is currently in works

The type of globalThis.Cloudflare.compatibilityFlags is Record<string, 'on'|'off'> ... is there anything more than that needed? If our types need to include all of the actual flag names then the type generation logic could fairly easily extract those, but I'm not super convinced it needs to be any more complicated than Record<string, 'on'|'off'>

... I expect that we'll need to change RTT to support dynamic type generation that is currently in works

How so?

can we ensure that these flags use an API that enable tree-shaking of the code, I don't believe that's the case now due to 'on' and 'off' values instead of using boolean type

Can you explain a bit more why the current proposed API doesn't work for tree shaking? Or, more usefully, propose an alternative API that would work better for tree shaking?

kentonv · 2024-08-16T15:09:33Z

I think we should go back to boolean values.

With the string on/off values, since it is unintuitive, people are likely to forget and write truthy checks instead, and then spend hours trying to figure out why workerd is ignoring them when they try to configure the flag to be off in their tests.

I don't think the string values solve the problem with negative flags. People will still write if (Cloudflare.compatibilityFlags.no_foo === 'on') { ... }, which will still do the wrong thing when running on an old version of workerd where the flag doesn't exist yet.

I think there are really two ways forward on that:

Don't include negative flags, only positive ones. Though that comes with its own confusion. Perhaps we want the negative flags to exist but throw an exception when accessed saying to please use the positive version instead?
Just live with it. If people are running old versions of workerd they may run into libraries that incorrectly check for newer features, and will have to file bugs to get those libraries to reverse the check.

I could be convinced either way.

are we intentionally not exposing the compatibility_date?

I would argue against exposing it, otherwise people might check against the date instead of against the specific flag, which would work most of the time but would be incorrect. Also, at present we don't actually have the compat date available on the edge -- it is consumed by the config service and converted to a boolean list before deployment.

dario-piotrowicz · 2024-08-16T15:27:53Z

What about something like this?

type CompatibilityFlags = {
  enabled: Set<string>;
  disabled: Set<string>;
};

interface Cloudflare {
  compatibilityFlag: CompatibilityFlags;
}

Seems very clear and not easy to get wrong to me, plus with unions it would also be very straightforward to get all possible flags

kentonv · 2024-08-16T15:31:47Z

@dario-piotrowicz That still has the same backwards-compatibility problem. If I write if (Cloudflare.compatibilityFlags.enabled.get("someNegativeFlag") { and run it on an old version of workerd it will incorrectly treat the flag as disabled. (Or if I write if (Cloudflare.compatibilityFlags.disabled.get("somePositiveFlag")) {.)

jasnell · 2024-08-16T16:14:33Z

PR updated with the changes:

Values switch back to boolean
The compatibilityFlags keys only include the enable flags whose values are explicitly true. See the comments in the updated test for more explanation.

src/workerd/api/global-scope.h

kentonv · 2024-08-19T15:56:54Z

The compatibilityFlags keys only include the enable flags whose values are explicitly true. See the comments in the updated test for more explanation.

Why? That means you can't check in to see if the workerd version knows about the flag.

Not necessarily saying I disagree with the change but was wondering what the motivation is. Sorry if I missed it somewhere in the long thread.

jasnell · 2024-08-19T21:27:45Z

@kentonv:

Why? That means you can't chec in to see if the workerd version knows about the flag.

#2527 (comment) Bullet point 1

I'd rather not have an API that throws on a known property getter as that carries its own DX hurdles.

kentonv · 2024-08-20T00:09:23Z

Why? That means you can't chec in to see if the workerd version knows about the flag.

#2527 (comment) Bullet point 1

I'd rather not have an API that throws on a known property getter as that carries its own DX hurdles.

Oh, but this isn't what I was asking for there.

By "negative flags" I didn't mean flags whose value is false, I meant flags declared with $compatDisableFlag (as opposed to $compatEnableFlag). Maybe I should have said "disable flags" instead of "negative flags". Disable flags are the ones that have issues with backwards-compatibility, because on old versions they will be undefined, which is falsy, even though the flag should logically be treated as true for old versions.

My suggestion is that we do not include disable-flags in the API at all, and instead tell people to test for the enable-flag, so that treating undefined as false is correct.

jasnell · 2024-08-20T15:37:12Z

Updated to include all enable flags.

jasnell · 2024-08-20T23:18:11Z

Rebased on master to resolve merge conflicts after the formatting change.

A simple built-in module and API for determining if a given compat flag is set. ``` const { compatibilityFlags } = globalThis.Cloudflare; console.log(compatibilityFlags['url_standard']); // 'on' or 'off' console.log(compatibilityFlags['url_original']); // 'on' or 'off' ```

kentonv

The logic regarding experimental flags seems odd to me but not enough to hold this up further.

jasnell · 2024-08-23T17:25:40Z

Ok, just waiting on a final review from @IgorMinar

Feedback addressed.

IgorMinar · 2024-08-27T18:23:19Z

My suggestion is that we do not include disable-flags in the API at all, and instead tell people to test for the enable-flag, so that treating undefined as false is correct.

Exactly!!! I'm reviewing the lastest, but I'm so happy that this is where we ended up. Full review soon.

IgorMinar

I like this version of flags api a lot! thank you @jasnell and all the reviewers for patiently iterating on this. I no longer have any concerns and I'm happy with this being merged.

I would still love to get an ok from the @andyjessop or someone (@penalosa, @petebacondarwin,
@lrapoport-cf ) from the wrangler team on the typescript support as I requested a while back.

I do, however, think the API is simple enough now and TS support should be non-controversial and unless they review quickly we don't need to wait for them and instead to a post-merge review.

IgorMinar · 2024-08-27T18:26:24Z

src/workerd/api/tests/compat-flags-test.js

+export const compatFlagsTest = {
+  test() {
+    // The compatibility flags object should be sealed, frozen, and not extensible.
+    throws(() => (compatibilityFlags.no_nodejs_compat_v2 = '...'));


now that we don't support disable flags, this test throws for a reason that is different than when the test was originally written, and is the same assertion as not_a_real_compat_flag. I suggest you either add a comment to say that you are explicitly testing for disable flags which should work as non-existent flags, or remove this line altogether.

IgorMinar · 2024-08-27T18:29:16Z

src/workerd/api/tests/compat-flags-test.js

+    ok(compatibilityFlags['nodejs_compat_v2']);
+    ok(compatibilityFlags['url_standard']);
+
+    ok(!compatibilityFlags['no_nodejs_compat_v2']);


this assertion doesn't prove that the value is undefined. I suggest that you test using in rather than by potentially coercing the return value via !

IgorMinar · 2024-08-27T18:30:46Z

src/workerd/api/tests/compat-flags-test.js

+
+    ok(!compatibilityFlags['no_nodejs_compat_v2']);
+    ok(!compatibilityFlags['url_original']);
+    strictEqual(compatibilityFlags['no_nodejs_compat_v2'], undefined);


oh, given this test, I'd remove all the ok(! assertions above as they only test that javascript works as expected.

Alternatively, it might be valuable to test for a lack of presence of the property descriptor via in or getOwnPropertyDescriptor.

IgorMinar · 2024-08-27T18:30:56Z

src/workerd/api/tests/compat-flags-test.js

+    ok(!compatibilityFlags['no_nodejs_compat_v2']);
+    ok(!compatibilityFlags['url_original']);
+    strictEqual(compatibilityFlags['no_nodejs_compat_v2'], undefined);
+    strictEqual(compatibilityFlags['url_original'], undefined);


the same as above

IgorMinar · 2024-08-27T18:31:30Z

src/workerd/api/tests/compat-flags-test.js

+
+    // Since we are not specifying the experimental flag, experimental flags should
+    // not be included in the output.
+    strictEqual(compatibilityFlags['durable_object_rename'], undefined);


could you instead assert that the property descriptor is missing? it's a more solid assertion.

IgorMinar · 2024-08-27T18:32:14Z

src/workerd/api/tests/compat-flags-test.js

+    // Since we are not specifying the experimental flag, experimental flags should
+    // not be included in the output.
+    strictEqual(compatibilityFlags['durable_object_rename'], undefined);
+    strictEqual('durable_object_rename' in compatibilityFlags, false);


ah.. you do, so I'd remove the first one as that only tests javascript's behavior and not our use of it.

IgorMinar · 2024-08-27T18:33:01Z

src/workerd/api/tests/compat-flags-test.js

+    strictEqual('durable_object_rename' in compatibilityFlags, false);
+
+    // If a flag does not exist, the value will be undefined.
+    strictEqual(compatibilityFlags['not-a-real-compat-flag'], undefined);


same as above, I'd remove

IgorMinar · 2024-08-27T18:35:07Z

src/workerd/api/tests/compat-flags-test.js

+    ok(keys.includes('url_standard'));
+    ok(!keys.includes('url_original'));
+    ok(!keys.includes('not-a-real-compat-flag'));
+  },


yeah. this is great!

jasnell requested review from a team as code owners August 13, 2024 18:36

jasnell requested review from a-robinson, hoodmane, IgorMinar, mhart and kentonv August 13, 2024 18:36

anonrig reviewed Aug 13, 2024

View reviewed changes

jasnell force-pushed the jsnell/compat-flags-module-take-2 branch from affe1e3 to 5c03173 Compare August 13, 2024 19:18

npaun reviewed Aug 13, 2024

View reviewed changes

0001-Implements-cloudflare-compatibility-flags-API.patch Outdated Show resolved Hide resolved

jasnell force-pushed the jsnell/compat-flags-module-take-2 branch from 47d97aa to 511ab4c Compare August 15, 2024 15:09

jasnell requested review from anonrig and npaun August 15, 2024 15:10

jasnell commented Aug 15, 2024

View reviewed changes

src/workerd/api/node/node.h Show resolved Hide resolved

anonrig approved these changes Aug 15, 2024

View reviewed changes

src/workerd/api/tests/compat-flags-test.js Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

jasnell force-pushed the jsnell/compat-flags-module-take-2 branch from 6ea42b8 to 8555fdf Compare August 15, 2024 16:47

This comment was marked as resolved.

Sign in to view

dario-piotrowicz reviewed Aug 15, 2024

View reviewed changes

src/workerd/api/global-scope.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

jasnell requested a review from dario-piotrowicz August 15, 2024 17:29

jasnell changed the title ~~Implements cloudflare:compatibility-flags API~~ Implements globalThis.Cloudflare.compatibilityFlags API Aug 15, 2024

dario-piotrowicz approved these changes Aug 15, 2024

View reviewed changes

IgorMinar previously requested changes Aug 16, 2024

View reviewed changes

src/workerd/api/tests/compat-flags-test.js Outdated Show resolved Hide resolved

src/workerd/api/tests/compat-flags-test.js Outdated Show resolved Hide resolved

jasnell requested review from IgorMinar, mhart, anonrig and dario-piotrowicz August 16, 2024 16:14

anonrig approved these changes Aug 16, 2024

View reviewed changes

src/workerd/api/global-scope.h Outdated Show resolved Hide resolved

anonrig approved these changes Aug 17, 2024

View reviewed changes

jasnell force-pushed the jsnell/compat-flags-module-take-2 branch from f7287af to 402c352 Compare August 20, 2024 15:02

jasnell force-pushed the jsnell/compat-flags-module-take-2 branch from 504a442 to a68b472 Compare August 20, 2024 23:17

jasnell force-pushed the jsnell/compat-flags-module-take-2 branch from a68b472 to 7543825 Compare August 20, 2024 23:28

kentonv approved these changes Aug 23, 2024

View reviewed changes

IgorMinar approved these changes Aug 27, 2024

View reviewed changes

jasnell merged commit 0563814 into main Aug 28, 2024
10 checks passed

jasnell deleted the jsnell/compat-flags-module-take-2 branch August 28, 2024 20:15

Implements globalThis.Cloudflare.compatibilityFlags API #2527

Implements globalThis.Cloudflare.compatibilityFlags API #2527

Conversation

jasnell commented Aug 13, 2024 • edited Loading

jasnell commented Aug 15, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

dario-piotrowicz left a comment

Choose a reason for hiding this comment

jasnell commented Aug 15, 2024

mhart commented Aug 15, 2024

jasnell commented Aug 15, 2024

dario-piotrowicz commented Aug 15, 2024

jasnell commented Aug 15, 2024

dario-piotrowicz commented Aug 15, 2024

IgorMinar left a comment • edited Loading

Choose a reason for hiding this comment

mhart commented Aug 16, 2024 • edited Loading

IgorMinar commented Aug 16, 2024

jasnell commented Aug 16, 2024 • edited Loading

kentonv commented Aug 16, 2024

dario-piotrowicz commented Aug 16, 2024

kentonv commented Aug 16, 2024 • edited Loading

jasnell commented Aug 16, 2024

kentonv commented Aug 19, 2024

jasnell commented Aug 19, 2024

kentonv commented Aug 20, 2024

jasnell commented Aug 20, 2024

jasnell commented Aug 20, 2024

kentonv left a comment

Choose a reason for hiding this comment

jasnell commented Aug 23, 2024

IgorMinar commented Aug 27, 2024

IgorMinar left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 13, 2024 •

edited

Loading

IgorMinar left a comment •

edited

Loading

mhart commented Aug 16, 2024 •

edited

Loading

jasnell commented Aug 16, 2024 •

edited

Loading

kentonv commented Aug 16, 2024 •

edited

Loading