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

Remove memoizer from UI #527

Merged
merged 9 commits into from
May 14, 2021
Merged

Remove memoizer from UI #527

merged 9 commits into from
May 14, 2021

Conversation

jnunemaker
Copy link
Collaborator

@jnunemaker jnunemaker commented May 10, 2021

Default rails and flipper setups will use memoizer so having ui include it causes the double memoize warning message, which can be confusing.

This removes the memoizer from UI and adds the backwards compatibility stuff we need to make it all work the same.

We can add a doc on how to add your own memoizer for just the ui:

run Flipper::UI.app { |builder|
  builder.use Flipper::Middleware::SetupEnv, some_flipper, env_key: "some_flipper"
  builder.use Flipper::Middleware::Memoizer, env_key: "some_flipper"
  # etc
}

We can also add a doc on how to add your own memoizer for just the api:

run Flipper::Api.app { |builder|
  builder.use Flipper::Middleware::SetupEnv, api_flipper, env_key: "api_flipper"
  builder.use Flipper::Middleware::Memoizer, env_key: "api_flipper"
  # etc
}

I'm open to doing something different. Just figured I'd throw out a first example to react to.

href #526

Default rails and flipper setups will use memoizer.

We can add a doc on how to add your own memoizer for just the ui.
@jnunemaker jnunemaker self-assigned this May 10, 2021
@jnunemaker jnunemaker requested a review from bkeepers May 10, 2021 12:24
If you aren't using default setup, you'll need to add this back in.

```
use Flipper::Api.app(flipper, env_key: "flipper_api") do |builder|
  builder.use Flipper::Api::Memoizer, flipper, env_key: "flipper_api"
end
```
Recommendation is to use setup env instead if you need anything other than the default instance of flipper.
@jnunemaker
Copy link
Collaborator Author

@bkeepers updated. Is this more like what you were picturing?

Copy link
Collaborator

@bkeepers bkeepers left a comment

Choose a reason for hiding this comment

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

Looks great!

@jnunemaker jnunemaker merged commit fe586af into master May 14, 2021
@jnunemaker jnunemaker deleted the ui-double-memoize branch May 14, 2021 23:57
@jnunemaker jnunemaker mentioned this pull request May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants