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

PublishContext should inherit from Subscription #159

Closed
mitar opened this issue Feb 27, 2016 · 11 comments
Closed

PublishContext should inherit from Subscription #159

mitar opened this issue Feb 27, 2016 · 11 comments

Comments

@mitar
Copy link
Contributor

mitar commented Feb 27, 2016

Because now is trying to mock Subscription, but not everything is there. Like _session, _subscriptionId, _idFilter and so on. I have quite some packages which extend publish handlers and use those internal values which will now not work inside SSR. Now, one option is that I manually make all of them work, or we just fix PublishContext to be more compatible with normal subscription.

@mitar
Copy link
Contributor Author

mitar commented Feb 27, 2016

See the issue which lead me to this: peerlibrary/meteor-reactive-publish#7

@arunoda
Copy link
Contributor

arunoda commented Feb 27, 2016

Yeah. That's a good idea,
Could you send me a PR. I'm happy to take this in.

You can get the Subscription class from here: https://github.com/meteorhacks/meteorx

@mitar
Copy link
Contributor Author

mitar commented Feb 27, 2016

Hm, the issue is that it is not enough just to extend it, but one should also configure all fields correctly. Not sure if this is doable. Like where to get session information.

@arunoda
Copy link
Contributor

arunoda commented Feb 28, 2016

Okay. That's a big project. Anyway, is there anyway we can build packages like yours without using those internals. If we can have a set of stuff we use, I think I can do something.

@mitar
Copy link
Contributor Author

mitar commented Feb 28, 2016

I think I found a way.

@arunoda
Copy link
Contributor

arunoda commented Feb 28, 2016

Awesome. Keep me posted.

@mitar
Copy link
Contributor Author

mitar commented Feb 28, 2016

See #160.

@arunoda
Copy link
Contributor

arunoda commented Mar 1, 2016

Hope we can close this and I merged @mitar's PR.

@arunoda arunoda closed this as completed Mar 1, 2016
@SachaG
Copy link

SachaG commented Mar 2, 2016

Does this mean the issue is also fixed for FlowRouter SSR? Or just Fast Render for now?

@arunoda
Copy link
Contributor

arunoda commented Mar 3, 2016

Just the FastRender. I'll do a release today.

@arunoda
Copy link
Contributor

arunoda commented Mar 3, 2016

@SachaG published a new version of FlowRouter SSR.

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