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

Only exporting withFeatures restricts use cases. #98

Closed
kennylavender opened this issue Feb 11, 2018 · 11 comments
Closed

Only exporting withFeatures restricts use cases. #98

kennylavender opened this issue Feb 11, 2018 · 11 comments
Assignees

Comments

@kennylavender
Copy link
Contributor

kennylavender commented Feb 11, 2018

WIP

Issue

Some projects using nextjs use loadGetInitialProps so that they can use getInitialProps in inner components. We currently only export withFeatures hoc. We can't add this functionality to withFeatures without making nextjs a dependency.

Applications can't make their own withFeatures hoc because we don't export a normal react component that they can build from.

Solution

Stuff will be here soon.

Dream Code

Some examples of the new code.

Creating a root component

import React from "react";
import { Provider } from "react-redux";
import { Features } from '@paralleldrive/react-feature-toggles';

const RootComponent = props => {
  const { store, history, initialFeatures,  query, children } = props;
  return (
    <Provider store={store}>
      <Features initialFeatures={initialFeatures} query={query}}>
        {children}
      </Features>
    </Provider>
  );
};

export default RootComponent;

Creating a HOC in some application that needs a custom hoc

import React, { Component } from 'react';
import { Features } from '@paralleldrive/react-feature-toggles';

const initialFeatures = [
  { name: 'foo', enabled: false },
  { name: 'bar', enabled: true },
];

const withFeatures = WrappedComponent => {
  class WithFeatures extends Component {
    static async getInitialProps(ctx) {
      const { req, query } = ctx;
      const subProps = await loadGetInitialProps(WrappedComponent, ctx);

      return {
        ...subProps,
        query
      };
    }
    render () {
      const query = this.props.query || this.context.query;
      return (
        <Features initialFeatures={initialFeatures} query={query}>
          <WrappedComponent {...this.props} />
        </Features>
      );
    }
  }
);

export default withFeatures;
@ericelliott
Copy link
Contributor

So Features would just provide the query/features context and pass through its props?

@kennylavender
Copy link
Contributor Author

kennylavender commented Feb 11, 2018

Yes, Features would be responsible for taking query and initialFeatures, then provide the correct enabled features to children through context.

I think we can just use props to provide Features with initialFeatures instead of configuring the component. Its idea I've been thinking about for a couple weeks.

That means that the enabled features will be more dynamic and will require a change to what the Features components stores in context. Because context can only be set once and should not be updated, we would instead store a function, like getEnabledFeatures() in context that the children components will call instead of just grabbing the data right from the context.

@kennylavender
Copy link
Contributor Author

If we want to make a quicker intermediate change, we could configure the Feature component so that its static like the current withFeatures hoc, that would allows us to make this change in an unbreaking way and we would just be exporting an extra component.

import React, { Component } from 'react';
import { features } from '@paralleldrive/react-feature-toggles';

const initialFeatures = [
  { name: 'foo', enabled: false },
  { name: 'bar', enabled: true },
];

const Features = createFeatures({initialFeatures});

const withFeatures = WrappedComponent => {
  class WithFeatures extends Component {
    static async getInitialProps(ctx) {
      const { req, query } = ctx;
      const subProps = await loadGetInitialProps(WrappedComponent, ctx);

      return {
        ...subProps,
        query
      };
    }
    render () {
      const query = this.props.query || this.context.query;
      return (
        <Features query={query}>
          <WrappedComponent {...this.props} />
        </Features>
      );
    }
  }
);

export default withFeatures;

@kennylavender kennylavender changed the title Only export withFeatures restricts use cases. Only exporting withFeatures restricts use cases. Feb 11, 2018
@ericelliott
Copy link
Contributor

ericelliott commented Feb 11, 2018

What if it was a render prop component? Would that add any useful flexibility? For one thing, we could easily reimplement a default withFeatures() HOC using it, and that could serve as an example of how to write a custom HOC... something like this:

const withFeatures = Component => (
  <Features initialFeatures={initialFeatures} query={query}>
    {props => <Component {...props} />}
  </Features>
);

Maybe all our OSS HOCs should be written this way, so we can provide both flexible reuse (render prop) and easy root level function composition (HOC).

@ericelliott ericelliott removed the bug label Feb 11, 2018
@kennylavender
Copy link
Contributor Author

Yeah that works too and is a bit more flexible! I like it

@ericelliott
Copy link
Contributor

BTW, don't worry about breaking changes. Break away if it gives us a better API. Breaking things early is better than breaking things later. =)

@kennylavender
Copy link
Contributor Author

great! I will get started then

@ericelliott
Copy link
Contributor

Is this solved now?

@kennylavender
Copy link
Contributor Author

No, the pr was only for the render prop component. Now that its merged I can continue

@ericelliott
Copy link
Contributor

👍 sorry for the delay. This fell of my radar. Feel free to ping if I block you on things! :)

@kennylavender
Copy link
Contributor Author

Closing as this component will be replaced by the component in #102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants