-
Notifications
You must be signed in to change notification settings - Fork 158
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
useFind and useSubscribe #332
Conversation
I added some initial tests for The test covers the basic I didn't add any tests to cover The only thing I think maybe I missed is that the Anyway, this is probably ready for either an alpha or beta release, IMHO. |
the "optional cursor" bit was because you cannot conditionally call a hook (while you can conditionally do a cursor.find() in a useTracker() callback). It's not necessarily tied to the isLoading state of a previous subscription, but could be just any condition (does some prop have a value ?) I'd think we do need this ability to conditionally return a cursor |
I'll add support for null return values - but it might be an anti-pattern to do conditional queries. I'll expand on that another time. |
Hey @CaptainN maybe this is out of the scope of this PR but I believe it's worth to mention. I'm working in a branch providing a way to use What does that mean? For example, if you need to call This is probably going to be optional in the client as we don't have Fibers in the client as well but if you want to keep your code isomorphic you should use the new async methods in the client as well. And why I'm bringing this here? Including an async call inside I didn't review the code of this PR yet to say how hard that would be or if this is possible keeping the same ideas but I decided to share this here as I'm working in the |
I agree it'll be challenging to support async/await in useTracker. Here is should be relatively easy to set it up as we need it. In |
I implemented and tested |
@CaptainN ready to move this to the next stage? |
Yep, ready for testing! Maybe a beta release? |
Let's resolve the conflicts and then we can do a beta release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @CaptainN 😄
I just suggested a new example in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need one slight addition in the documentation and should be ready to go. I'm testing this and so far no problems.
Hi @CaptainN sorry for the late feedback but shouldn't we have a way to skip a subscribe and even a find? At least I have one useFind that I implemented myself and in some cases I need to skip the subscription because I don't have all the params and as we can't call hooks conditionally it's necessary to provide a way to skip inside the hook. My example: const { data: modules } = useFind({
publicationName: 'modules',
arg: { modeName },
skip: !modeName,
find: () => ModulesCollection.find({}, { sort: { order: 1 } }),
}); My implementation (it's not a package, just the code in one of my apps): export const useFind = ({
publicationName,
arg,
find,
skip,
shouldSkip,
dataReturnWhenLoading,
}) =>
useTracker(() => {
if (skip || (shouldSkip && shouldSkip())) {
return {
data: [],
skipped: true,
};
}
const handler = Meteor.subscribe(publicationName, arg);
if (!handler.ready()) {
return {
loading: true,
data: dataReturnWhenLoading || undefined,
};
}
const result = find();
// is findOne? so no fetch
return { data: result.fetch ? result.fetch() : result };
}); How could I do the same using the hooks from this package? How could I skip the subscription call when |
You can't conditionally call a subscription - but |
My point is not to block this PR, it is just to understand how common this could be. I would say this is pretty common. But if you disagree, ok :) |
I think it might be less common than it seems if you are organizing your code with smaller purpose built components, the way the two hooks - But like I said, it's easy enough to fall back to We could refactor to allow the use of a factory, instead of or in addition to the non-factory hook signature we use now, if it's too confusing the way it is. |
No, it is not confusing. I was asking because it was a need that I had just a few days ago and I would like to confirm how common do we think this is. thank you! btw, @StorytellerCZ is probably going to publish a new version of this package soon including new hooks. |
I really don't know how common it would be, but I do wonder whether it's part of Meteor's philosophy to reduce friction when coding, and if so, maybe we should add the factory option to |
So actually, |
…rmanent one is created in useTracker.
Prepare for 2.4-beta.2 release.
I have bumped the versions for next beta release @filipenevola |
Published [email protected]. |
I've refactored my internal import { Meteor } from 'meteor/meteor';
import { getLanguage } from './languages';
import { useTracker } from 'meteor/react-meteor-data';
const subscribeCall = (publicationName, arg) => {
const argWithLanguage = { ...(arg || {}), language: getLanguage() };
return Meteor.subscribe(publicationName, argWithLanguage);
};
export const useFind = ({
publicationName,
arg,
find,
skip,
shouldSkip,
dataReturnWhenLoading,
}) =>
useTracker(() => {
if (skip || (shouldSkip && shouldSkip())) {
return {
data: [],
skipped: true,
};
}
if (publicationName) {
const handler = subscribeCall(publicationName, arg);
if (!handler.ready()) {
return {
loading: true,
data: dataReturnWhenLoading || undefined,
};
}
}
const result = find();
// is findOne? so no fetch
return { data: result.fetch ? result.fetch() : result };
}); to use the new hooks and they are working perfectly, now I'm calling my hook import { getLanguage } from './languages';
import { useFind, useSubscribe } from 'meteor/react-meteor-data';
export const useData = ({
publicationName,
arg,
find,
skip,
shouldSkip,
dataReturnWhenLoading,
deps = [],
}) => {
const argWithLanguage = { ...(arg || {}), language: getLanguage() };
const isSkip = skip || (shouldSkip && shouldSkip());
const isLoading = useSubscribe(
isSkip ? null : publicationName,
argWithLanguage
);
const result = useFind(find, deps);
if (isLoading()) {
return {
loading: true,
data: dataReturnWhenLoading || undefined,
};
}
// is findOne? so no fetch
return { data: result.fetch ? result.fetch() : result };
}; You can see it in action here. |
That's a great example of functional composition. Love it. I'm not sure |
It's working but I believe I just have one use case for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
v2.4.0 is published now. @CaptainN do you want to start the blog post for this version? |
BTW, as always, thank you for the great work. |
This PR adds
useFind
anduseSubscribe
as discussed and prototyped in thenew-hooks
branch (PR #298).The hooks are basically done. They just need docs and tests.
TODO: