Support setting GKs in RecoilEnv #2078

impl · 2022-10-24T20:42:42Z

In #366 (comment), it was suggested that we could possibly use RecoilEnv to dynamically enable GKs:

Would be interesting if someone wanted to do a PR for mapping GK flags here with the new RecoilEnv.js mechanism to more easily dynamically adjust features without rebuilding..

Well, I wanted to try out the transition support in the same way OP of that issue wanted memory management, so I decided to give the implementation a try: this change adds a new property to RecoilEnv, RECOIL_GKS_ENABLED_UNSTABLE, which can be used to enable certain GKs at runtime. When the property is updated, its changes are proxied to the underlying GK implementation.

impl

Once y'all are happy with the API, I can push up a docs PR too! :)

impl · 2022-10-24T20:43:40Z

packages/recoil/core/Recoil_RecoilEnv.js


 export type RecoilEnv = {
  RECOIL_DUPLICATE_ATOM_KEY_CHECKING_ENABLED: boolean,
+  RECOIL_GKS_ENABLED_UNSTABLE: string[],


I made this name _UNSTABLE because I figured the GKs could change over time, but maybe this API itself won't? Not sure if that totally makes sense...

Yeah, I would drop the _UNSTABLE here I guess if we are happy with the API. Are you? Maybe call it RECOIL_GKS_ENABLE? Would it also make sense to have a RECOIL_GKS_DISABLE option?

I've been using this patch in my codebase and it suits my needs so I'm happy with it :) I'll rename it to RECOIL_GKS_ENABLE. Regarding adding a ...DISABLE, I think it raises some questions about the semantics of setting the values in that case:

If you remove a GK from the ...ENABLE list, does it disable it? Does it add it to ...DISABLE implicitly? (And vice-versa...)

If a GK is present in both ...ENABLE and ...DISABLE, which one wins?

I think my preference would be to just leave ...ENABLE and have it be authoritative, i.e. setting it simply changes the list of enabled GKs to whatever it contains.

Thinking out loud, the only thing that's not great then is that environment variable users would need to list all the default GKs if they want to override it. Perhaps when the environment variable is set, we include the default GKs in the list anyway unless the user all provides a magic -defaults value or something? Or we could have two env vars, ...ENABLE and ...REPLACE or something?

I was expecting the RECOIL_GKS_ENABLE to act as an override enable for a particular GK if a GK is present in that list. That means no ability to disable a default GK. No need to make things overly complex or add DISABLE at this time unless there's a real need, just trying to think through the ramifications of the API..

Got it, that makes sense to me. I think in that case we just need to make the list effectively "append-only" so that if we include the default GKs in it they can't be removed. Then we could decide to allow removals in the future if we wanted (or implement a separate ...DISABLE), which would be an API-safe change.

I guess an alternative could be instead of trying to enforce append-only or subscribing to changes to update the GKX pass/fail the gkx() wrapper itself could check the environment variable for overrides each time so it's always safe to add/remove enable overrides and the dynamic checks will update at runtime.

impl · 2022-10-24T20:45:10Z

packages/recoil/core/Recoil_RecoilEnv.js

+        // Remove old GKs set in the environment, filter out any GKS that are
+        // already set by default, and set new GKs.
+        obj[prop].forEach(gkx.setFail);
+        obj[prop] = value.filter(gk => !gkx(gk));


This is sort of weird because it can potentially remove values from the array before it gets set (so if you check the value after you set it, it might be different than what you set), but it prevents the situation where someone could potentially "enable" and then accidentally disable a built-in enabled GK when they update the property.

I'm a little confused why it's necessary to call setFail() for all the enabled GKs here? Wouldn't the following gkx() call then always fail?

Seems the default GKs should also be added to this environment object for introspection.

This is basically to work around the default GKs not being present, so if we move it around so that it's in the shared package I don't think it would be necessary.

ok, so moving it really sound a lot cleaner then!

impl · 2022-10-24T20:46:23Z

packages/recoil/core/Recoil_RecoilEnv.js

-const env: RecoilEnv = {
-  RECOIL_DUPLICATE_ATOM_KEY_CHECKING_ENABLED: true,
-};
+const env: RecoilEnv = new Proxy(


I couldn't think of a better way to implement this than using a proxy (or moving RecoilEnv into the shared package and having Recoil_gkx reference it, but that seemed like a bigger refactor), but if there is one, I'd be happy to take a suggestion to improve it!

This does seem a bit overly complex. Moving to shared would be totally fine. Actually, that would be safer to ensure the GK module is updated before any actual GK checks are made, depending on the order the modules are run.

impl · 2022-10-24T20:46:42Z

packages/recoil/core/Recoil_RecoilEnv.js

+    return;
+  }
+
+  set(sanitizedValue.split(/\s*,\s*/));


Is a comma a good separator for the environment variable?

Comma sounds great. But, also allow whitespace as well.

facebook-github-bot · 2022-10-25T19:32:40Z

@wd-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

drarmstr · 2022-10-31T22:27:23Z

@impl @wd-fb - Sorry for the delayed review, I was out sick...

facebook-github-bot · 2022-11-01T21:20:04Z

@impl has updated the pull request. You must reimport the pull request before landing.

impl · 2022-11-01T21:23:28Z

OK! I think I've made some progress here. Now gkx() reads directly from RecoilEnv.RECOIL_GKS_ENABLE, so the expected usage would be either:

Set the RECOIL_GKS_ENABLE environment variable to whatever GKs you want enabled, either comma- or space-separated, and they will be added to the list of default GKs.
Add to the RecoilEnv.RECOIL_GKS_ENABLE set in your code before you set up a RecoilRoot, e.g. RecoilEnv.RECOIL_GKS_ENABLE.add('recoil_some_gk');

Let me know what you think!

impl · 2022-11-01T21:28:06Z

packages/shared/util/Recoil_RecoilEnv.js

 };

 const env: RecoilEnv = {
  RECOIL_DUPLICATE_ATOM_KEY_CHECKING_ENABLED: true,
+  RECOIL_GKS_ENABLE: new Set([


I noticed this one is present tense but the other existing one is past tense. Maybe this should be RECOIL_GKS_ENABLED after all?

makes sense. consistency is good.

This change adds a new property to RecoilEnv, RECOIL_GKS_ENABLED, which can be used to enable certain GKs at runtime. We update the OSS gkx() implementation to refer to this property directly, and as a result we have to move the RecoilEnv to the shared package (which also conveniently ensures that RecoilEnv properties and gkx() are initialized together).

facebook-github-bot · 2022-11-02T00:28:35Z

@impl has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-11-02T18:58:51Z

@wd-fb has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…#2090) Summary: When I run `yarn test`, I got a test case failure from facebookexperimental#2078 that says ``` You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/en/configuration.html Details: Recoil/packages/shared/util/__tests__/Recoil_RecoilEnv-test.js:13 import { IS_INTERNAL } from 'Recoil_TestingUtils'; ``` This PR is a fix for that Pull Request resolved: facebookexperimental#2090 Differential Revision: D41105610 Pulled By: drarmstr fbshipit-source-id: a3f54b2f7eaf679aee6b9b3505a52ce8217812a5

…#2090) Summary: When I run `yarn test`, I got a test case failure from facebookexperimental#2078 that says ``` You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/en/configuration.html Details: Recoil/packages/shared/util/__tests__/Recoil_RecoilEnv-test.js:13 import { IS_INTERNAL } from 'Recoil_TestingUtils'; ``` This PR is a fix for that Pull Request resolved: facebookexperimental#2090 Differential Revision: D41105610 Pulled By: drarmstr fbshipit-source-id: 7e55df8ed1257d6548c71716a21ae69d168aa2a6

Summary: When I run `yarn test`, I got a test case failure from #2078 that says ``` You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/en/configuration.html Details: Recoil/packages/shared/util/__tests__/Recoil_RecoilEnv-test.js:13 import { IS_INTERNAL } from 'Recoil_TestingUtils'; ``` This PR is a fix for that Pull Request resolved: #2090 Reviewed By: wd-fb Differential Revision: D41105610 Pulled By: drarmstr fbshipit-source-id: 3ef1e854fbe75167d2c4d5289297aa07ebce9611

Summary: In facebookexperimental/Recoil#366 (comment), it was suggested that we could possibly use `RecoilEnv` to dynamically enable GKs: > Would be interesting if someone wanted to do a PR for mapping GK flags here with the new `RecoilEnv.js` mechanism to more easily dynamically adjust features without rebuilding.. Well, I wanted to try out the transition support in the same way OP of that issue wanted memory management, so I decided to give the implementation a try: this change adds a new property to `RecoilEnv`, `RECOIL_GKS_ENABLED_UNSTABLE`, which can be used to enable certain GKs at runtime. When the property is updated, its changes are proxied to the underlying GK implementation. Pull Request resolved: facebookexperimental/Recoil#2078 Test Plan: CI, unit tests Reviewed By: drarmstr Differential Revision: D40688490 Pulled By: wd-fb fbshipit-source-id: 8a748d453e22782c6122a8d5047155019c5d3a1c

Summary: When I run `yarn test`, I got a test case failure from facebookexperimental/Recoil#2078 that says ``` You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/en/configuration.html Details: Recoil/packages/shared/util/__tests__/Recoil_RecoilEnv-test.js:13 import { IS_INTERNAL } from 'Recoil_TestingUtils'; ``` This PR is a fix for that Pull Request resolved: facebookexperimental/Recoil#2090 Reviewed By: wd-fb Differential Revision: D41105610 Pulled By: drarmstr fbshipit-source-id: 3ef1e854fbe75167d2c4d5289297aa07ebce9611

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 24, 2022

impl commented Oct 24, 2022

View reviewed changes

drarmstr added the enhancement New feature or request label Oct 31, 2022

impl force-pushed the env-gkx branch from 791a32d to 2ef88cc Compare November 1, 2022 21:20

impl commented Nov 1, 2022

View reviewed changes

impl force-pushed the env-gkx branch from 2ef88cc to ab92f08 Compare November 2, 2022 00:28

impl mentioned this pull request Nov 3, 2022

[docs] Document the RECOIL_GKS_ENABLED env var #2085

Merged

facebook-github-bot closed this in 0a6b314 Nov 3, 2022

Gumichocopengin8 mentioned this pull request Nov 6, 2022

test: fix import error that causes test failure #2090

Closed

impl deleted the env-gkx branch May 14, 2023 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting GKs in RecoilEnv #2078

Support setting GKs in RecoilEnv #2078

impl commented Oct 24, 2022

impl left a comment

impl Oct 24, 2022

drarmstr Oct 31, 2022

impl Nov 1, 2022

drarmstr Nov 1, 2022

impl Nov 1, 2022

drarmstr Nov 1, 2022

impl Oct 24, 2022

drarmstr Oct 31, 2022

impl Nov 1, 2022

drarmstr Nov 1, 2022

impl Oct 24, 2022

drarmstr Oct 31, 2022

impl Oct 24, 2022

drarmstr Oct 31, 2022

facebook-github-bot commented Oct 25, 2022

drarmstr commented Oct 31, 2022

facebook-github-bot commented Nov 1, 2022

impl commented Nov 1, 2022

impl Nov 1, 2022

drarmstr Nov 1, 2022

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

Support setting GKs in RecoilEnv #2078

Support setting GKs in RecoilEnv #2078

Conversation

impl commented Oct 24, 2022

impl 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

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

facebook-github-bot commented Oct 25, 2022

drarmstr commented Oct 31, 2022

facebook-github-bot commented Nov 1, 2022

impl commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022