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

Meteor crashes if app doesn't depend on webapp #2168

Closed
jsep opened this Issue May 21, 2014 · 17 comments

Comments

Projects
None yet
4 participants
@jsep
Copy link

jsep commented May 21, 2014

Hello,

This is actually 3 bugs that need to be solved in order for run-command not to crash. Those are:

  1. Missing no-ops in the livedata package if there is no webapp
  2. ServiceConfiguration backwards compatibility code needs to be moved to service-configuration package from accounts-base
  3. Missing check that Meteor.server exists in accounts-base - this one cannot be reproduced without solving the above

We will publish shortly an app to reproduce all bugs as well as issue a pull request to fix all of them.

@jsep

This comment has been minimized.

Copy link
Author

jsep commented May 21, 2014

This is the app that reproduces the bugs:

https://github.com/spacejamio/meteor-issue-2168

Run it with:

meteor run-command packages/program

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented May 22, 2014

There is another manifest related bug if the package DOES depend on webapp / standard-app-packages, but I don't see why a command line program should start a web server in the first place, so we didn't even create a bug for it.

@glasser

This comment has been minimized.

Copy link
Member

glasser commented May 22, 2014

run-command has never been documented, never quite worked, and is currently slated to be removed in 0.9.0 (it's gone on the packaging branch).

@glasser

This comment has been minimized.

Copy link
Member

glasser commented May 22, 2014

That said if the other issues are replicable without run-command (eg an app without standard-app-packages) I'd be interested in learning about it.

@glasser glasser closed this May 22, 2014

@jsep jsep changed the title Meteor run-command crashes if run-command package doesn't depend on webapp Meteor crashes if app doesn't depend on webapp May 22, 2014

@jsep

This comment has been minimized.

Copy link
Author

jsep commented May 22, 2014

We were able to reproduce the bugs in a regular meteor app not depending on webapp, using plain old meteor run. Steps to reproduce:

git clone https://github.com/spacejamio/meteor-issue-2168.git
cd meteor-issue-2168/app
meteor --once

We also changed the bug title to reflect it. We will issue another pull request shortly.

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Jun 3, 2014

@glasser, did u have a chance to review pull request?

@glasser

This comment has been minimized.

Copy link
Member

glasser commented Jun 10, 2014

OK, so the fundamental issue here is that ever since we added server-to-server DDP to Meteor in 0.6.2, livedata has been a sort of ill-defined package. In browsers, it provides a DDP client; in Node, it provides both a DDP client and a DDP server, and the latter requires you to be hosting a web app. Of course, sometimes you want a DDP client in Node without hosting a web app, so there's a very hacky check of "is the webapp package loaded" to try to guess what you want.

We know it should be split into two packages: ddp-client and ddp-server. ddp-client would provide a DDP client in both the browser and in Node; ddp-server would provide a server in Node only. Since accounts-base needs to define methods, it would depend on ddp-server which would itself depend on webapp. This would fix the error.

On the other hand, this would mean that including accounts-base implies you're running a webapp, and your meteor --once won't end until stopped explicitly. That might not be what you want? Honestly, I don't know why you want to be able to use accounts-base in Node without running a server, since accounts-base is mostly a package that provides RPCs.

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Jun 10, 2014

@glasser, you are right, there is no reason to use accounts-base in a cli app. It was due to our package dependencies, where one of our base packages depended on accounts-base and our cli package / app depended on this base package where we came across the bug. Our oversight. Sorry for the waste of your time.

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Dec 16, 2014

@glasser this became an issue again. We created an mcli package and a starter cli app, and any person that wants to use a meteor package in their cli app that depends directly or indirectly on accounts-base, won't be able to use those packages in their cli app, which is a huge limitation for the community.

I've created the following simplest app (without our mcli package) to illustrate the problem:

https://github.com/practicalmeteor/meteor-issue-2168

Just clone, run meteor --once and see the problem. meteor remove accounts-base-dependable, and it goes away.

We already had a pr that solved it a long ago, can I just update it to break the circular reference?

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Dec 17, 2014

@glasser do you want me to seperate this into 2 seperate issues and prs?

  1. Deal with missing noop / stub methods in ddp if webapp is not present? methods, publish, onConnection
  2. Deal with accounts-base, service-configuration circular dependencies?
@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Dec 30, 2014

Trying again. Would really love to resolve this.

@glasser

This comment has been minimized.

Copy link
Member

glasser commented Jan 9, 2015

Underlying issue split out into #3452.

@glasser glasser added the Project:DDP label Jan 9, 2015

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Jan 20, 2015

@glasser , splitting ddp into 2 is a major task. This can simply be solved by first dealing with noop methods in case webapp doesn't exist, and then solve the circular dependency, which is a really easy task as well. Can I start by creating a pr (or 2) for those 2?

@glasser

This comment has been minimized.

Copy link
Member

glasser commented Jan 21, 2015

You can send a PR, but please do read our https://github.com/meteor/meteor/wiki/Contributing-to-Meteor#feature-requests policy to set expectations.

@rbabayoff

This comment has been minimized.

Copy link
Contributor

rbabayoff commented Jan 21, 2015

@glasser, that works for me. Can I assume I have you on board or should I post to meteor-core first?

@glasser

This comment has been minimized.

Copy link
Member

glasser commented Jan 21, 2015

Sure, feel free to post it now, but that doesn't mean any promises about it finding its way up the priority list.

@abernix

This comment has been minimized.

Copy link
Member

abernix commented Jul 15, 2016

Closing as this doesn't seem relevant anymore. The reproductions seem to operate (generally) fine (without the error previously shown) in the latest Meteor, but I'm not even sure it's appropriate to test this anymore. Happy to reopen if anyone feels differently. 😄

@abernix abernix closed this Jul 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment