Skip to content
This repository has been archived by the owner on Nov 11, 2023. It is now read-only.

feat: add displayName to Context component #166

Closed
wants to merge 0 commits into from
Closed

feat: add displayName to Context component #166

wants to merge 0 commits into from

Conversation

alfdocimo
Copy link

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
Copy link
Contributor

stereobooster commented Nov 4, 2019

Hi, @alfdocimo. Welcome to the project.

I'm not quite sure about this PR:

  • there are a lot of changes, can you maybe make 2 separate PRs one for documentation and one for the feature?
  • initial idea was actually set displayName to restful-react and not to provide way to set it, so when you debug you know that those contexts are part of restful-react.

@TejasQ
Copy link
Contributor

TejasQ commented Nov 18, 2019

@alfdocimo are you still interested in implementing this?

@alfdocimo
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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

@alfdocimo
Copy link
Author

Hiya! any updates on this? 😊

@TejasQ
Copy link
Contributor

TejasQ commented Nov 25, 2019

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 reduxjs/react-redux#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 alfdocimo closed this Dec 4, 2019
@alfdocimo
Copy link
Author

Oh!

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

@alfdocimo
Copy link
Author

Opened #195 😉

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

Successfully merging this pull request may close these issues.

3 participants