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

Conflict with collection-hooks package #2

Closed
dburles opened this issue Jan 15, 2014 · 21 comments
Closed

Conflict with collection-hooks package #2

dburles opened this issue Jan 15, 2014 · 21 comments

Comments

@dburles
Copy link
Owner

dburles commented Jan 15, 2014

Meteor.users.helpers is undefined

@dburles
Copy link
Owner Author

dburles commented Jan 16, 2014

@matb33 wondering if you might have some ideas on a work around, or if the issue might somehow be addressed within collection-hooks internals? :)

@dburles
Copy link
Owner Author

dburles commented Jan 21, 2014

workaround, place collection-hooks after collection-helpers within .meteor/packages file

@matb33
Copy link

matb33 commented Jan 23, 2014

Sorry for not responding any earlier, I looked quickly for a few minutes but didn't have a chance to look deeper. .helpers should be defined -- are you putting .helpers on the prototype? It should carry along if done that way

@dburles
Copy link
Owner Author

dburles commented Jan 23, 2014

No problem, indeed helpers is applied to the prototype. I think I'll dig into the issue a bit more today and let you know

@dburles
Copy link
Owner Author

dburles commented Jan 23, 2014

Still not really sure!, though it's fairly easy to reproduce, just place collection-hooks before collection-helpers in your .meteor/packages file

@dburles
Copy link
Owner Author

dburles commented Feb 1, 2014

@matb33 Do you have any ideas?

@matb33
Copy link

matb33 commented Feb 2, 2014

I haven't had time to look. I possibly may have a chance today (fingers crossed)

@mizzao
Copy link

mizzao commented Mar 12, 2014

Yet another problem that may be solved if one just registers a weak dependency from collection-hooks (this package, Collection2, etc)

@dburles
Copy link
Owner Author

dburles commented Mar 12, 2014

Not sure exactly what you mean?

On Wednesday, 12 March 2014, Andrew Mao notifications@github.com wrote:

Yet another problem that may be solved if one just registers a weak
dependency from collection-hooks (this package, Collection2, etc)

Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-37369506
.

@mizzao
Copy link

mizzao commented Mar 12, 2014

If a package specifies a weak dependency on another package, that package always gets loaded before this one. So instead of running in circles trying to make either load order work, I suggest coordinating with other package authors to just agree to have one specify a weak dependency on the other, especially when one or both of the packages is modifying prototypes or built-in functions that are prone to pathological behavior.

This was also mentioned in Meteor-Community-Packages/meteor-collection-hooks#24 although @matb33 was kind of against it at the time ;)

@dburles
Copy link
Owner Author

dburles commented Mar 12, 2014

Yeah I guess it seems okay however it feels a little bit band aid in this
case. However the main issue still remains, that even in your main
application if you're using collection-hooks and nothing else you'll
encounter this issue with attaching prototype functions to collections.

Not really sure if there's a way around that. What do you think?

On Wednesday, 12 March 2014, Andrew Mao notifications@github.com wrote:

If a package specifies a weak dependencyhttp://docs.meteor.com/#writingpackageson another package, that package always gets loaded before this one. So
instead of running in circles trying to make either load order work, I
suggest coordinating with other package authors to just agree to have one
specify a weak dependency on the other, especially when one or both of the
packages is modifying prototypes or built-in functions that are prone to
pathological behavior.

This was also mentioned in Meteor-Community-Packages/meteor-collection-hooks#24https://github.com/matb33/meteor-collection-hooks/pull/24although
@matb33 https://github.com/matb33 was kind of against it at the time ;)

Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-37370892
.

@matb33
Copy link

matb33 commented Mar 12, 2014

When I last looked at this, I couldn't for the life of me remember why I was even touching Meteor.users. My tests passed with all that code commented out. I haven't been very active with collection-hooks due to workload but I will eventually look at this issue

@dburles
Copy link
Owner Author

dburles commented Mar 12, 2014

Hey @mattb33 it wasn't really obvious to me why it was happening either,
thanks though for checking it out

On Wednesday, 12 March 2014, Mathieu Bouchard notifications@github.com
wrote:

When I last looked at this, I couldn't for the life of me remember why I
was even touching Meteor.users. My tests passed with all that code
commented out. I haven't been very active with collection-hooks due to
workload but I will eventually look at this issue

Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-37371662
.

@mizzao
Copy link

mizzao commented Mar 12, 2014

@matb33 wasn't it because Meteor.users was instantiated before collection-hooks was loaded, and therefore it wasn't going to get hooked in the constructor?

In any case the problem seems to be https://github.com/matb33/meteor-collection-hooks/blob/master/collection-hooks.js#L115

By the way: this reminds me, collection-hooks needs to declare a weak dependency on accounts-base otherwise there is a chance that it gets loaded first and Meteor.users does not get hooked.

@matb33
Copy link

matb33 commented Mar 12, 2014

@mizzao oh man of course... the weak dependency makes it a non-issue. Back when I first wrote collection hooks, the package system didn't have weak dependency support so that was my workaround.

Good call! I'm going to do this now before I forget

@matb33
Copy link

matb33 commented Mar 12, 2014

I jumped the gun, forgot that weak dependency loads in the opposite order I needed. While I'm in the code I'll see what I can do regardless

@mizzao
Copy link

mizzao commented Mar 12, 2014

Hmm...you want accounts-base to be loaded after collection-hooks? What's the idea behind that?

@matb33
Copy link

matb33 commented Mar 12, 2014

I wish... then when Meteor.users = new Meteor.Collection was called, it would have our prototype in place.

As it stands, the fix I'm implementing right now uses the discouraged __proto__ technique. It's the only way we can re-assign a prototype to an existing instance

@mizzao
Copy link

mizzao commented Mar 12, 2014

I just opened an issue requesting support for a reverse weak dependency in the new package system.

@matb33
Copy link

matb33 commented Mar 12, 2014

@mizzao nice, thank you. I just released a new version of collection-hooks with the IE8 fix from @davidworkman9 and the __proto__ trick. It has performance implications that I don't grok, but it's only used on Meteor.users.

@dburles can you check to see if Meteor.users.helpers is now available to you?

@dburles
Copy link
Owner Author

dburles commented Mar 13, 2014

hey @matb33 just tried it out, works great here :)

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

No branches or pull requests

3 participants