Skip to content
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

Make collection helpers work even if other transforms are present #59

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

Conversation

ephemer
Copy link

@ephemer ephemer commented Feb 9, 2016

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
Copy link
Owner

dburles commented Feb 10, 2016

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
Copy link
Author

ephemer commented Feb 10, 2016

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
Copy link
Owner

dburles commented Feb 10, 2016

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
Copy link
Author

ephemer commented Feb 11, 2016

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
Copy link
Owner

dburles commented Feb 11, 2016

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
Copy link
Owner

dburles commented Feb 11, 2016

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

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

TAPi18n(Books);

@ephemer
Copy link
Author

ephemer commented Feb 12, 2016

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
Copy link

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
Copy link
Owner

dburles commented Feb 14, 2016

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
Copy link

Thanks for the tip, seems to work a charm 👌

@dburles
Copy link
Owner

dburles commented Feb 14, 2016

@mtchllbrrn great!

@mtchllbrrn
Copy link

@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
Copy link
Owner

dburles commented Feb 16, 2016

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

@SimonSimCity
Copy link

SimonSimCity commented Sep 20, 2017

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 meteor/meteor-feature-requests#183

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.

4 participants