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

Add mongo-dev-server package #8853

Merged
merged 11 commits into from Jul 26, 2017

Conversation

Projects
None yet
4 participants
@zimme
Contributor

zimme commented Jun 27, 2017

Only start the MongoDB server if this package is present in the project.

For now I've added mongo-dev-server as a dependency of mongo, do we want to do this?

A lot of packages rely on the meteor package and when using Meteor as a build system you might wanna use packages like static-html or dynamic-import but you might still not want a local mongodb instance started.

I'm thinking we could remove mongo-dev-server as a mongo dependency and use the upgrades thing to just add this package to all current meteor projects on upgrade and add it to the default packages when creating a new project.

The way this PR is now results in MONGO_URL being set to 'no-mongo-server' if mongo-dev-server isn't present and MONGO_URL hasn't been manually set. This will prevent Meteor from trying to connect to a local mongo server even if mongo is present in the project. I don't know if this have any other negative effects.

This is a first step at resolving meteor/meteor-feature-requests#31.

Add mongo-dev-server package
Only start the MongoDB server if this package
is present in the project.
@zimme

This comment has been minimized.

Contributor

zimme commented Jun 27, 2017

Turned out that the changes in #8769 was enough for this to work. There was no need to do the same for .meteor/versions to cover this use-case. i.e. --extra-packages mongo-dev-server/mongo will trigger the starting of mongodb.

@zimme

This comment has been minimized.

Contributor

zimme commented Jun 28, 2017

I'll have a look at the self tests, some might fail if they expect MongoDB without adding mongo.

@zimme zimme changed the title from Add mongo-dev-server package to [Work in progress] Add mongo-dev-server package Jul 2, 2017

@hwillson

Thanks @zimme, this looks great! I've added a few small review comments, but I'll take another look after the tests are passing. Thanks again!

# mongo-dev-server
[Source code of released version](https://github.com/meteor/meteor/tree/master/packages/mongo-dev-server) | [Source code of development version](https://github.com/meteor/meteor/tree/devel/packages/mongo-dev-server)
***

This comment has been minimized.

@hwillson

hwillson Jul 10, 2017

Member

Would you mind adding one or two sentences describing the package in the README?

This comment has been minimized.

@zimme

zimme Jul 10, 2017

Contributor

I can do this.

});
Package.onUse(function (api) {
api.addFiles('info.js', 'server');

This comment has been minimized.

@hwillson

hwillson Jul 10, 2017

Member

Maybe consider using api.mainModule here instead?

This comment has been minimized.

@zimme

zimme Jul 10, 2017

Contributor

I'll have a look.

@@ -34,7 +34,8 @@ Package.onUse(function (api) {
'diff-sequence',
'mongo-id',
'check',
'ecmascript'
'ecmascript',
'mongo-dev-server',

This comment has been minimized.

@hwillson

hwillson Jul 10, 2017

Member

Relating to your comment:

I'm thinking we could remove mongo-dev-server as a mongo dependency and use the upgrades thing to just add this package to all current meteor projects on upgrade and add it to the default packages when creating a new project.

I like this idea - Not forcing mongo-dev-server as a dependency, and using an upgrader instead, gets my vote!

This comment has been minimized.

@zimme

zimme Jul 10, 2017

Contributor

I think I'd need help with how to add this to the upgrader instead.

This comment has been minimized.

@hwillson

hwillson Jul 10, 2017

Member

Sure thing - I haven't had to build one myself, but as far as I understand things, you'll be looking to add your changes in tools/upgraders.js. The process seems to be pretty well documented in that file - you'll be adding a new upgrade name / section, and you can test things out by running meteor admin run-upgrader myupgradername. The 1.5-add-dynamic-import-package upgrader section in that file might be a good one to copy / start from.

});
const packageMap = self.projectContext.packageMap;
const hasMongoDevServerPackage = packageMap && packageMap.getInfo('mongo-dev-server') != null;

This comment has been minimized.

@hwillson

hwillson Jul 10, 2017

Member

Would you mind switching to use !== just to make sure we're matching the rest of the source.

This comment has been minimized.

@zimme

zimme Jul 10, 2017

Contributor

I used != to match both null and undefined. But if getInfo always returns null when not found then it shouldn't be a problem using !==.

This comment has been minimized.

@hwillson

hwillson Jul 10, 2017

Member

Thanks @zimme, it's probably okay to leave it then. I personally prefer to not use lenient equality / non-equality checks on null / undefined, as when going back over old code I always end up questioning whether the leniency was intended or not ... 🙂.

@benjamn

After talking about this in the issue triage club (including @hwillson), the team is comfortable with the dependency on the dev-only mongo-dev-server package, rather than some other approach like an upgrader.

@zimme

This comment has been minimized.

Contributor

zimme commented Jul 12, 2017

Just wanted to give you guys a heads up, I'm heading off on vacation this Friday for 2 weeks and I don't think I'll have time to finish this up before that.

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 13, 2017

Nooo, you can't leave!! Seriously though, no problem at all @zimme - thanks for getting things this far. I'll aim to make the remaining small changes, over the next few days (schedule permitting of course - you might find this PR waiting for you when you get back 😉). Have a great 🏖 !

@benjamn benjamn added this to the Release 1.5.2 milestone Jul 19, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 22, 2017

We should be all set here now - LGTM!

@abernix

This comment has been minimized.

abernix commented on 7203e03 Jul 25, 2017

@hwillson Was this commit unrelated to this PR or meant to be included in meteor#8933?

This comment has been minimized.

hwillson replied Jul 25, 2017

@abernix I meant to include it as part of this PR, as I was making adjustments to help get the tests pass here first. I then realized that we need more adjustments, so I created meteor#8933 separately. Since I had already committed this here though, I just left it. Let me know if you think it should be moved. Thanks!

@abernix abernix merged commit 4d37a05 into meteor:devel Jul 26, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abernix

This comment has been minimized.

Member

abernix commented Jul 26, 2017

LGTM, thanks!

abernix added a commit that referenced this pull request Jul 26, 2017

Add mongo-dev-server package (#8853)
* Add mongo-dev-server package

Only start the MongoDB server if this package
is present in the project.

* Small layout/formatting adjustments; updated README.

* Allow tests using fake-mongod to start (fake) Mongo.

* Adjust test stdout matching to be less sensitive to ordering.

* Add `mongo-dev-server` History.md entry.

* Remove mongo start check since the tested for error prevents mongo startup.

* Remove README traling whitespace.

* Bump mongo package version.

benjamn added a commit that referenced this pull request Aug 3, 2017

Revert "Add mongo-dev-server package (#8853)"
This reverts commit 4d37a05.

After git bisecting between origin/release-1.5 and origin/release-1.5.2, I
identified this commit as the culprit in recent failures of the modules
test app: https://circleci.com/gh/meteor/meteor/4857#tests/containers/3

Note that the modules test app seems to be failing only on Linux, and it
does pass reliably with this commit reverted. It must have something to do
with Mongo failing to start, and thus the "App running at" message never
appears, but I don't have a good theory why that might be.

The command to run just the modules test app is

  meteor self-test --history 1000 'modules - test app'

@zimme @hwillson @abernix any ideas?

hwillson added a commit to hwillson/meteor that referenced this pull request Aug 9, 2017

Add mongo-dev-server package (meteor#8853)
Only start the MongoDB server if this package
is present in the project.

This reverts commit 565281e.

benjamn added a commit that referenced this pull request Aug 10, 2017

@zimme zimme deleted the zimme:zimme/mongo-dev-server branch Sep 5, 2017

@zimme zimme changed the title from [Work in progress] Add mongo-dev-server package to Add mongo-dev-server package Sep 5, 2017

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