contiamo / restful-react

A consistent, declarative way of interacting with RESTful backends, featuring code-generation from Swagger and OpenAPI specs 🔥
MIT License
1.87k stars 109 forks source link

feat: add displayName to Context component #166

Closed alfdocimo closed 4 years ago

alfdocimo commented 5 years ago

Why

Add capabilities to display the name of the Provider and the Consumer as per #107

Also, fixed some typos on the README file 😁

Note / Question:

I was wondering where to add the Unit test for these changes as I see that Context is used in various files but there's not a Context.test.js 😅

Sorry if my lint made some other changes ):

stereobooster commented 5 years ago

Hi, @alfdocimo. Welcome to the project.

I'm not quite sure about this PR:

TejasQ commented 4 years ago

@alfdocimo are you still interested in implementing this?

alfdocimo commented 4 years ago

Hey there! Sorry, I've been busy. I am 😁, It's just that maybe this PR should be separated as @stereobooster mentioned. Also, just to clarify, displayName should just have "restful-react" then?


export const Context = React.createContext<Required<RestfulReactProviderProps>>({
  base: "",
  parentPath: "",
  resolve: (data: any) => data,
  requestOptions: {},
  onError: noop,
  queryParams: {},
  displayName: "restful-react", // is this necesary?
});

export interface InjectedProps {
  onError: RestfulReactProviderProps["onError"];
}

export default class RestfulReactProvider<T> extends React.Component<RestfulReactProviderProps<T>> {
  public render() {
    const { children, ...value } = this.props;
    return (
      <Context.Provider
        value={{
          onError: noop,
          resolve: (data: any) => data,
          requestOptions: {},
          parentPath: "",
          queryParams: value.queryParams || {},
          displayName: "restful-react",
          ...value,
        }}
      >
        {children}
      </Context.Provider>
    );
  }
}

Thanks again, and I hope to make these changes asap (:

stereobooster commented 4 years ago

Also, just to clarify, displayName should just have "restful-react" then?

Yeah, maybe RestfulReactContext to be more consistent in naming (because components are typically camel-case, except the base ones).

Thanks for taking time.

alfdocimo commented 4 years ago

Updated! 🙏 (Could use some help on creating unit tests for this) Also will create another PR for typos in the readme

alfdocimo commented 4 years ago

Hiya! any updates on this? 😊

TejasQ commented 4 years ago

Hey @alfdocimo! Thanks for putting the time into this. I think we may not be on the same page about the problem we're trying to solve.

What we want to do is not add a configurable displayName prop, but instead, give the contexts a default display name in order to help debugging when using the React dev tools. See https://github.com/reduxjs/react-redux/issues/1461 and https://hackernoon.com/improving-component-names-in-react-developer-tools-4894247510c5 for reference.

In this case, what needs to be done is adding

  static displayName = "RestfulProviderContext";

above this line, and then everything should be OK.

What questions do you have?

alfdocimo commented 4 years ago

Oh!

Thanks for the article @TejasQ, mind = blown 🤯. Yeah, definitely not what I understood. Will add this and create a PR 👍

alfdocimo commented 4 years ago

Opened #195 😉