search

trojanowski / react-apollo-hooks

Use Apollo Client as React hooks
MIT License
2.41k stars 110 forks source link

Merging into react-apollo? #40

Open fbartho opened 5 years ago

fbartho commented 5 years ago

Were you planning on or Would you object to somebody taking this repo and trying to contribute this back to react-apollo?

I love what you've started here, and I want to help!

trojanowski commented 5 years ago

Hi @fbartho

How do you think this repo could be merged with react-apollo? What should be added or changed?

I wasn't very active here recently, but it should improve. I'm going to actively maintain this library and use it in a new project. I'd like the hooks to be added to the official react-apollo, and I'd be happy to help with the implementation. But I also think it would make sense for this library to exist even after that - as something like "react-apollo-lite" - e. g. for new projects where you don't need non-hooks API.

In the next releases (in addition to bugfixes) I'd like to add:

I'd appreciate any help. Please let me know what do you think.

fbartho commented 5 years ago

Hiya! Based on my readings, if we do our jobs right, places that care about code-file-size can use tree-shaking to shake out the parts they don't use. So… if we do a good job, I'd almost say we "just" need to contribute the hooks bits back into core!

I do agree that the TypeScript-first code is probably a requirement for getting this into core.

Besides that, each of the other features you describe sound useful and nice to have.

Were you thinking about "other" things that you wanted to add to this "react-apollo-lite" that wouldn't be accepted by react-apollo (core)?

trojanowski commented 5 years ago

Were you thinking about "other" things that you wanted to add to this "react-apollo-lite" that wouldn't be accepted by react-apollo (core)?

Not right now. So I agree that if we add hooks to react-apollo and can use just them without adding too much additional stuff to the app bundles, we will depreciate that library and recommend to use the official hooks.

rosskevin commented 5 years ago

I'm a comitter on react-apollo (but not an Apollo employee) and have some thoughts. @trojanowski please dm me on the Apollo slack. I do think there is an opportunity to merge this work there, but things can move quickly here unconstrained. I'm interested to debate pros/cons and perhaps adding a couple contributors here to help move forward until the core team is ready to look at hooks.

JoviDeCroock commented 5 years ago

The first thing that would need to be pushed forward in react-apollo to make this happen is the new context API.

Possibly you would get the argument that this would increase bundle size but you could say that hooks are a major (3.0) and implement the HOC and render props with the hooks you made. That would limit the bundle size increase to a bare minimum (I would dare to say even reduce it since the need for classes are totally gone then).

So for me this would be a really ideal step towards 3.0

trojanowski commented 5 years ago

@trojanowski please dm me on the Apollo slack.

@rosskevin done

I do think there is an opportunity to merge this work there, but things can move quickly here unconstrained.

I'm going to make two breaking changes here, and after that make PR-s and discuss them before merging them into master (maybe with an exception for obvious bugfixes and documentation updates), so you will be able to warn us if something would cause potential problems with merging with react-apollo. That breaking changes will be:

New features with the highest priority for me are:

rosskevin commented 5 years ago

All of this sounds good. The only thing I want to get done sooner than later is rollup bundles for esm and cjs, in-line with the strategy already merged in the upcoming apollo-client release.

I'll help move hooks forward here, following semver to give good signals/care for the users of this lib do what we can to (hopefully) prepare a good foundation for consideration by the core maintainers for the next major react-apollo release. I'm just a contributor, so I don't know how the core maintainers will ultimately decide, but in my experience they have been very open to contributions.

A compatibility layer would be good here, but not sure yet whether we should venture into creating hoc/render callback wrappers for the hooks here or not. I think the next major of react-apollo could expose hoc/render callback patterns that simply delegate to a hook implementation. We did the same in react-i18next.

trojanowski commented 5 years ago

The only thing I want to get done sooner than later is rollup bundles for esm and cjs, in-line with the strategy already merged in the upcoming apollo-client release.

@rosskevin I'm just curious what are actual benefits of using rollup here. The one I'm aware of is a possibility of preparing separate production and development bundles (like it's done in React - especially important for Node.js), but Apollo libraries don't do that.

rosskevin commented 5 years ago

@trojanowski primarily to avoid hidden interop problems mixing index and path imports introduce - it's the easiest way (and a nasty problem).

FezVrasta commented 5 years ago

I read that the react-apollo team was having concerns regarding the amount of different "patterns" exposed by the react-apollo package (HOCs, render-prop, hooks).

Thinking about it, everything you need to provide HOCs and render-prop APIs are Hooks.

You can easily use the Apollo Hooks to build the other two variants. That would greatly simplify the code-base complexity.

Naive example:

const Query = ({ query, children, ...options }) => children(useQuery(query, options));
const withQuery = ({ query, ...options }) => Component => {
  const res = useQuery(query, options);
  return <Component {...res} />;
}
rosskevin commented 5 years ago

Hey @FezVrasta can you link to that discussion? I agree, hooks should probably be the implementation.

FezVrasta commented 5 years ago

This is the comment (specifically "the ugly"): https://github.com/apollographql/react-apollo/issues/2539#issuecomment-464025309

my idea won't solve the "mental overhead", but at least it will be easy to maintain.

danielkcz commented 5 years ago

@FezVrasta It's all nice and shiny, but it would be a breaking change and would require React 16.8+ to be used by everyone. Either way, you should probably open a new issue in react-apollo as hooks are used in this one all the way ;)

rosskevin commented 5 years ago

@FredyC this idea is not new so it should not yield a new discussion. My understanding is that hooks are logical as the foundation. hwillson has valid concerns mostly about legacy and avoiding api churn for users. Some of us like to move faster and it probably makes sense for this hooks project to remain separate for a bit as he suggests.

I stopped following that conversation due to the plethora of responses.

The concern is that if you have all those ways to access, it is an overhead from both docs, issues, and general maintenance. While the implementation is in one place, that doesn't stop new users from filing issues thinking it is different because they are using x to access.

I think react-apollo has a high noise to signal ratio, something I tried to help with but ultimately a reduction in api surface area would certainly help most.