Skip to content

Astronomy support #13

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ochicf
Copy link

@ochicf ochicf commented Oct 6, 2016

Added compatibility with Astronomy.

@ochicf ochicf mentioned this pull request Oct 6, 2016
@SachaG
Copy link
Contributor

SachaG commented Oct 8, 2016

So all you need for Astronomy support is for the collection prop to work with both an object or a collection?

@ochicf
Copy link
Author

ochicf commented Oct 8, 2016

Well, that was my intention initially: that the collection prop could be either an object (meteor collection) or a function (astronomy class). That worked well since the astronomy class, though being a function, had the same methods that the Meteor collection that it wraps.

While checking the code for possible problems, I saw that each join's collection property was already accepting both an object or a function, the latter being invoked immediately and assigning its return value as the actual collection. This made me change my mind about my approach and take this as the way to go, mainly because:

  • it would be a consistent way for a collection to be received both in collection and joins.collection props
  • it is more versatile than taking the received function as a collection (since the function as a collection works only for Astronomy classes)
  • the passed function can return an Astronomy class to be as assigned as the collection

To exemplify all cases:

import { Foo, Bar } from '/lib/collections';
import { FooAstroClass, BarAstroClass } from '/lib/classes';

console.log(typeof Foo); // object
console.log(typeof Bar); // object
console.log(typeof FooAstroClass); // function
console.log(typeof BarAstroClass); // function

const listContainerProps = {
    joins: [],
};

// ## Initial approach ##
// accept both object or function and treat them as a collection

// directly assign astro class to collection prop works well
listContainerProps.collection: FooAstroClass;

listContainerProps.joins.push({
    localProperty: 'refBarId',
    joinAs: 'bar',
    // passing an astronomy collection throws a TypeError because it
    // is invoked as a function to retrieve a collection but it is
    // a class constructor
    collection: BarAstroClass,
});

// ## Final approach ##
// imitate how join.collection works for collection props

// pass a function that retrieves an astronomy class
listContainerProps.collection: () => FooAstroClass;

listContainerProps.joins.push({
    localProperty: 'refBarId',
    joinAs: 'bar',
    // consistent with how collection prop was passed
    collection: () => BarAstroClass,
});

I've though about doing some type checking before calling the received function, in order to allow an Astronomy class to be passed directly, but I'm not sure if it's a good solution. The most generic way I can think of is to check if the received collection has the expected interface for a collection (find and findOne methods) and treat it as such if so. If not, invoke it if it's a function.

So, the behavior would be:

// both meteor collection and astroclass can be passed directly
// because both have the expected collection interface
listContainerProps.collection = Foo;
listContainerProps.collection = FooAstroClass;

// a function without collection interface is invoked
listContainerProps.collection = () => Foo;
listContainerProps.collection = () => FooAstroClass;

If you like this last idea I can try to implement it.

@SachaG
Copy link
Contributor

SachaG commented Oct 9, 2016

I don't know much about Astronomy so it's hard for me to tell what's best… I'm happy to trust you with this though :)

In fact if you're interested in contributing to the package, I would be happy to make you a maintainer. I'll be honest, I feel like the future of pub/sub in Meteor is Apollo + GraphQL, so I don't think I'll invest a ton of time in this package anymore.

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

Successfully merging this pull request may close these issues.

2 participants