Make collection helpers work even if other transforms are present

[ephemer]

We are using TAPi18nDB, which uses transforms, and we also want to continue using collection helpers. Here's a way of having both. Cheers.

[dburles]

Hey @ephemer it's intentional to not extend upon any existing transform function to avoid potentially very difficult to trace bugs. I'd suggest manually adding methods by supplying your own transform function when initialising a collection, e.g:

  Author = function(doc) { return _.extend(this, doc); };

  Author.prototype.fullName = 'Charles Darwin';

  Authors = new TAPi18n.Collection('authors', {
    transform: function(doc) { return new Author(doc); }});

The tapi18n API really should be a mixin rather than a constructor IMO.

[ephemer]

Hey, I agree about the TAPi18n API

Not sure what kind of bugs you could avoid by manually adding the transform compared to just doing it in helpers. Do you have a specific example of that? Functionally, what you posted here seems pretty much identical to the changes I made in the PR. I don't know how the transform-in-the-constructor syntax works internally, but I can't imagine it being significantly different.

The only thing I could think of is that you might get into some kind of weird reactivity loop of the helpers are accessing each other in a strange way. That's more of a meteor/tracker issue than one specific to the helpers implementation though.

[dburles]

Well you wouldn't avoid bugs by doing that, but since collection-helpers simply won't work, you can instead do it manually :)

One bug example would simply be overwriting a method, if you had a collection with an existing transform that added a method called foo and then you were to write MyCollection.helpers({ foo: 'bar' }); it would be overwritten. This could potentially be a bad thing for the existing package relying on that helper to do something entirely different.

It's one of those things where there's a small chance of it being a problem but I'm still leaning towards it being more correct to not allow it for the sake of this package and those who use it. By applying the transform on your own without the use of this package, you're then on your own when it comes to any potential conflicts.

[ephemer]

Yes, overriding one transform's methods with another's with the same name could be a problem. I think it's a shame to miss out on this helper+transform functionality entirely though.

What if I update the PR to check the transformed document's prototype to see if the methods you want to add already exist and throw an error in that case? Specifically that would happen instead of a blind _.extend(this, doc)

[dburles]

Yeah I've considered that, though I'm not entirely convinced that's the way to go either. To be honest it seems it's pretty rare to have a conflict like the one with tap-i18n-db. I think this is really the only one I can recall. If its API could apply its own transform after the collection has been instantiated (mixin) it would solve the problem.

[dburles]

Books = new Mongo.Collection('books');

Books.helpers({
   // ...
});

TAPi18n(Books);

[ephemer]

I've updated the pull request with checks to ensure helpers will never overwrite pre-existing transforms, with updated and expanded tests. I've also published ephemer:[email protected] in the meantime with this functionality. Feel free to take it or leave it. Cheers

[mtchllbrrn]

I'm interested in being able to define my own transforms, as well. For instance, I have a collection that needs to store a Mongo query object, which will sometimes include keys beginning with '$'. This is not allowed by Mongo, so I've been using hooks to serialize (JSON.stringify) these object on insert, and a transform to parse (JSON.parse) it as an object when fetched.

I want this to occur invisibly, without having to manually invoke a helper on insert or on fetch. Unfortunately, I can't find a way to make this work with this package. Additionally, I'm getting errors from ephemer's collection-helpers package mentioned above:

Error: This only happens when your Collection has an existing transform that accesses non-initialised state internally and then you try to add helpers via Collection.helpers({}). [Error while adding helpers to your Collection]

Here's the transform I'd like to perform:

Campaigns = new Mongo.Collection('campaigns', {
  transform: (doc) => {
    doc.query = JSON.parse(doc.query);
    return doc;
  },
});

[dburles]

Hey @mtchllbrrn there's no easy way to do that with this package however, it's simple enough to achieve. Try the following (with this package):

Test = new Mongo.Collection('test');

Test.helpers({
  foo: 'bar'
});

Test._transform = doc => {
  doc.query = JSON.parse(doc.query);
  return new Test._helpers(doc);
};

[mtchllbrrn]

Thanks for the tip, seems to work a charm :ok_hand:

[dburles]

@mtchllbrrn great!

[mtchllbrrn]

@dburles: I spoke too soon. It looks like the operation is occurring as expected, but I receive the following error in the server console:

Exception in setTimeout callback: TypeError: undefined is not a function
    at Segments._transform (models/Segments.js:254:10)
    at packages/minimongo/wrap_transform.js:28:1
    at Object.Tracker.nonreactive (packages/tracker/tracker.js:589:1)
    at SynchronousCursor.wrapped [as _transform] (packages/minimongo/wrap_transform.js:27:1)
    at SynchronousCursor._nextObject (packages/mongo/mongo_driver.js:1003:20)
    at SynchronousCursor.forEach (packages/mongo/mongo_driver.js:1020:22)
    at SynchronousCursor.map (packages/mongo/mongo_driver.js:1030:10)
    at SynchronousCursor.fetch (packages/mongo/mongo_driver.js:1054:17)
    at Cursor.(anonymous function) [as fetch] (packages/mongo/mongo_driver.js:869:44)
    at MongoConnection.findOne (packages/mongo/mongo_driver.js:776:56)

I'm using exactly the same syntax as what you posted above.

[dburles]

@mtchllbrrn Hmm try see if you can reproduce the bug in an isolated application, otherwise there's too many variables to consider

[SimonSimCity]

I'd really like to see this solved. It plays a significant role when I try to fix the problem of identifying which subscription publicated a document. See https://github.com/meteor/meteor-feature-requests/issues/183