Refactor the models code to the models.js file. #164

Closed
wants to merge 1 commit into from

3 participants

@wbzyl

Removed duplicated model code from the todos example.

Now the todos code looks like wordplay code.

--Włodek Bzyl

@TomWij TomWij commented on the diff May 27, 2012
examples/todos/models.js
+// "name" : "Meteor Principles",
+// "_id" : "8f0f0323-9a52-424b-9a24-cacf0662f0ed"
+// }
+
+Todos = new Meteor.Collection("todos");
+// {
+// "list_id" : "8f0f0323-9a52-424b-9a24-cacf0662f0ed",
+// "text" : "Data on the Wire",
+// "timestamp" : 1337970786875,
+// "tags" : [
+// "Simplicity",
+// "Better UX",
+// "Fun"
+// ],
+// "_id" : "81b3c12a-5897-4da0-b7ac-11cbe2f7834e"
+// }
@TomWij
TomWij added a note May 27, 2012

I think it would be better if people learn to do console.log(Todos.findOne({})); to inspect how a document looks like in an example such that they learn to inspect / debug, because documenting the collections like this is non-standard. Or at least clarify that this is the structure used...

@wbzyl
wbzyl added a note May 27, 2012

Agreed. What about adding a new section:

Things to try out

1. Inspect Todos and Lists on the browser console:

Todos.findOne()
Lists.findOne()

2. Inspect the saved data on the meteor mongo console:

$ meteor mongo
db.todos.find()
db.lists.find()
@TomWij
TomWij added a note May 27, 2012

Showing them that there's something like meteor mongo can be quite helpful knowledge. Good idea. +1

@wbzyl
wbzyl added a note May 27, 2012

Adding info about the help command could be useful too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@TomWij TomWij commented on the diff May 27, 2012
examples/todos/client/todos.js
@@ -1,9 +1,5 @@
// Client-side JavaScript, bundled and sent to client.
-// Define Minimongo collections to match server/publish.js.
-Lists = new Meteor.Collection("lists");
-Todos = new Meteor.Collection("todos");
-
@TomWij
TomWij added a note May 27, 2012

In both client and server, it might be useful to specify that this is specified in the shared models.js file. Because this is one of the very first examples, it might be unclear when people see that Lists / Todos is suddenly being used but not know where it is defined.

@wbzyl
wbzyl added a note May 27, 2012

What about adding section Make some changes:

Make some changes

  • Move duplicate code from client/todos.js and server/publish.js files to the new models.js file.
@TomWij
TomWij added a note May 27, 2012

Not sure about that. Just wanted to make them aware that there's both options for collections that are shared.

@wbzyl
wbzyl added a note May 27, 2012

I would like to close this pull request. The better place for changes is this page. What about adding
two new section Make some changes, Things to try out and moving your comments there?

@TomWij
TomWij added a note May 27, 2012

Please note that I'm not one of the main contributors of Meteor. Just suggesting things to think about...

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

1. What about adding a note to the page about the purpose of the Meteor.publish code from the server/publish.js file. I think that Meteor.subscribe and Meteor.autosubscribe are somehow connected with the Meteor.publish code. Am I right?

2. Is it possible to move Helpers for in-place editing from todos.js file to a new file? What is the best place for that file? A separate folder? or exisitng client folder.

3. Why the server code:

Meteor.startup(function () {
  Backbone.history.start({pushState: true});
});

was put in the client/todos.js file? Is it possible to mve it to server/publish.js?

Any clarification on above comments would be appreciated.

@jonathanKingston

Backbone isn't implemented server side its for the client routing handling pages routing to the next page and so on. However speaking to the main contributors they will tell you its only an interim solution and one where a unified approach across client and server is needed as part of Meteor.

@jonathanKingston

Meteor.autosubscribe subscribes to all the changes in the collections... if you want to use publish you need to turn auto subscribe off and publish each collection yourself. This is really just a beginners package to help them have all the server collections on the client side however further down the line you will want to limit some collections by the amount you publish and also certain fields.

@wbzyl

@jonathanKingston Many thanks for the clarifications.

@debergalis
Meteor Development Group member

I'm going to hold off on Todos changes until we've finished schemas and validations.

@debergalis debergalis closed this Jun 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment