Skip to content

Setup package for feature flags #4617

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.

Sign up for GitHub

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

Merged
devongovett merged 2 commits into from
Jun 2, 2023
Merged

Setup package for feature flags #4617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
486743d
Setup package for feature flags
devongovett Jun 1, 2023
4230c3f
Separate functions for each flag
devongovett Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/@react-stately/flags/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# @react-stately/flags

This package is part of [react-spectrum](https://github.com/adobe/react-spectrum). See the repo for more details.
13 changes: 13 additions & 0 deletions packages/@react-stately/flags/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* Copyright 2023 Adobe. All rights reserved.
* This file is licensed to you under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. You may obtain a copy
* of the License at http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under
* the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/

export * from './src';
30 changes: 30 additions & 0 deletions packages/@react-stately/flags/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"name": "@react-stately/flags",
"version": "3.0.0-alpha.1",
"description": "Spectrum UI components in React",
"license": "Apache-2.0",
"main": "dist/main.js",
"module": "dist/module.js",
"types": "dist/types.d.ts",
"exports": {
"types": "./dist/types.d.ts",
"import": "./dist/import.mjs",
"require": "./dist/main.js"
},
"source": "src/index.ts",
"files": [
"dist",
"src"
],
"sideEffects": false,
"repository": {
"type": "git",
"url": "https://github.com/adobe/react-spectrum"
},
"dependencies": {
"@swc/helpers": "^0.4.14"
},
"publishConfig": {
"access": "public"
}
}
22 changes: 22 additions & 0 deletions packages/@react-stately/flags/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright 2023 Adobe. All rights reserved.
* This file is licensed to you under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. You may obtain a copy
* of the License at http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under
* the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/

Copy link
Member

@snowystinger snowystinger Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think flags should only be set once, especially since it's not state, so it likely won't cause the app to dump and reload.

So I think we should export a singleton instance, and the setter should only set once, and then freeze whatever object we store in the singleton, then we use a getter of some sort to check. This could help if any of our flags end up with dependencies on each other.

This will also prevent us from changing exports routinely

Do we want to allow more than booleans? I'm trying to think if any other data would be useful here. Fortunately this isn't a rollout mechanism, so we don't need to worry about dates/users/etc.

Copy link
Member Author

@devongovett devongovett Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's start simple. we can always do more fancy things later but I don't think it's necessary to get too complex for now. I don't think modifying exports is necessarily bad. In fact ESM is specifically designed to allow this. It also enables flags for components that you don't use in your app to be completely tree shaken. Freezing will prevent flags from being set in different places within the app. For example, you might have one file that renders a Table and that one is what sets the flag for table, and another file which sets a flag for menu. I was about to update the API to allow that actually. I do think making it only allow enabling and not disabling makes sense though.

Copy link
Member

@snowystinger snowystinger Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, happy to start simple and nix most of those things. However, I think changing the flags, in any direction, after the very first step of loading your app, is a bad idea. For instance, if you have two tables, and you render the first one expecting no features, and then you render the second one and turn on the features. Now the first table may be in a bad state if say, the feature flag was choosing between two hooks or suddenly added a hook or different state objects. And to make it a little worse, you may not know that the first Table is in a bad state for a long time, maybe it won't manifest a problem until it re-renders, or maybe you'd need to click on a cell. It can be very difficult to debug.

Copy link
Member Author

@devongovett devongovett Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know, that's why I wrote "The flags also need to be set before anything is rendered in the app or things may break unexpectedly, so libraries like quarry should not be enabling flags, they should be enabled in apps." in the description.

But freezing the object wouldn't prevent this. You could still set no flags before rendering, and only set them once afterward which would have the same problem. Anyway, what I have now is equivalent to this since you can now only set them from false to true and not the other direction.

Copy link
Member

@snowystinger snowystinger Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had two questions come in today alone where people did not read the documentation.

If they must all be set at the same time, I think that would increase the odds that people do it in the right place. It's not foolproof, for sure, but I think it could be helpful in deterring misuse.

I went looking to see if we could detect if the code was run during a react lifecycle or during render, but couldn't find anything for that. Also discarded doing it from our Provider because the aria/stately packages don't require our provider.

We can still improve on it, so I've approved. In the big picture, these are fairly small issues I have. I only worry about support, as always, 😢

export let unavailableMenuItems = false;
export let tableNestedRows = false;

export function enableUnavailableMenuItems() {
unavailableMenuItems = true;
}

export function enableTableNestedRows() {
tableNestedRows = true;
}