Allow QueryManager to intercept hook functionality #11617
Conversation
🦋 Changeset detectedLatest commit: 682aa45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
| // @ts-expect-error Cannot assign to 'useBackgroundQuery' because it is a function.ts(2630) | ||
| useBackgroundQuery = wrapped; |
There was a problem hiding this comment.
TS really doesn't like this.
On the other hand, this is valid JS - see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
I don't understand why bundling |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
The tree-shaking is solved now - I had some help from @Andarist to find a pattern that works here. This pattern is admittedly quite ugly, but it works along all bundlers and should be very solid. @jerelmiller could you please take another look? |
| useBackgroundQuery = makeHookWrappable( | ||
| "useBackgroundQuery", | ||
| (_, options) => | ||
| useApolloClient(typeof options === "object" ? options.client : undefined), | ||
| _useBackgroundQuery as any | ||
| ); | ||
|
|
||
| return useBackgroundQuery.apply(null, arguments as any); |
There was a problem hiding this comment.
Perhaps I'm missing something obvious or its been a long travel day, but couldn't you just set this to any variable name and call it? Any reason you need to override the original function?
| useBackgroundQuery = makeHookWrappable( | |
| "useBackgroundQuery", | |
| (_, options) => | |
| useApolloClient(typeof options === "object" ? options.client : undefined), | |
| _useBackgroundQuery as any | |
| ); | |
| return useBackgroundQuery.apply(null, arguments as any); | |
| const uBQ = makeHookWrappable( | |
| "useBackgroundQuery", | |
| (_, options) => | |
| useApolloClient(typeof options === "object" ? options.client : undefined), | |
| _useBackgroundQuery as any | |
| ); | |
| return uBQ.apply(null, arguments as any); |
or if if you wanted to do it all inline
return makeHookWrappable(
"useBackgroundQuery",
(_, options) =>
useApolloClient(typeof options === "object" ? options.client : undefined),
_useBackgroundQuery as any
).apply(null, arguments as any);There was a problem hiding this comment.
This is based on a babel pattern:
function _extends() {
_extends = Object.assign
? Object.assign.bind()
: function (target) {
for (var i = 1; i < arguments.length; i++) {
var source = arguments[i];
for (var key in source) {
if (Object.prototype.hasOwnProperty.call(source, key)) {
target[key] = source[key];
}
}
}
return target;
};
return _extends.apply(this, arguments);
}
var a = _extends({}, b);Essentially, it's a lazy override pattern, so makeHookWrappable wouldn't have to be called again in the future.
But, one sleep later, I agree with you that this can be simplified - we are not on a hot path here (something is seriously wrong if we ever are), so we might as well optimize a bit more for readability and amount of code.
I still prefer the first approach we had, overriding the hooks after creation, as that way we didn't have to touch their contents. But as we don't have that option, I believe what I've ended up doing now should be one of the simpler possible solutions. Can you please take another look?
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
The (unminified) diff for bundling > var wrapperSymbol = Symbol.for("apollo.hook.wrappers");
> function wrapHook(hookName, useHook, clientOrObsQuery) {
> var queryManager = clientOrObsQuery["queryManager"];
> var wrappers = queryManager && queryManager[wrapperSymbol];
> var wrapper = wrappers && wrappers[hookName];
> return wrapper ? wrapper(useHook) : useHook;
> }
>
---
495a506,512
> return wrapHook(
> "useReadQuery",
> _useReadQuery,
> unwrapQueryRef(queryRef)["observable"]
> )(queryRef);
> }
> function _useReadQuery(queryRef) { |
| const removeComments = cleanup({ | ||
| comments: ["some", /#__PURE__/, /#__NO_SIDE_EFFECTS__/], | ||
| }); |
There was a problem hiding this comment.
We don't strictly need this here anymore in this specific case, but it would be good to have this in our bundling config anyways, so I'll leave it here.
phryneas
changed the title
phryneas
changed the title
This is an alternative to #11615
I considered adding this as a property of the global object, or just a property on context, but it seemed most fitting that an Apollo Client instance can add logic to the hooks. This was an argument especially against context, as a hook can be called with a
clientargument and shouldn't be behaving differently in that case.The benefit of this approach in general is that hooks can be overwritten without having to actually import the hooks - making them tree-shakable.
With the old approach, if
useClientwould be overwritten, it would always need to be bundled, even if the hook might not be used at all in a modern code base.