jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
605 stars 66 forks source link

Documentation fixes and improvement ideas #489

Closed arggh closed 7 years ago

arggh commented 8 years ago

I was just skimming through the new Astronomy docs before upgrading to v2.0 and found some inconsistencies. I would have made a pull request about the changes if I'd found the proper repository to do so with: it seems that meteor-astronomy-docs is not used anymore?

Any way, here goes:

1. About Find-events

An event object passed to the beforeFind event handler contains three additional properties selector, options and result. So, in this event handler you can additionally modify result of a find operation.

=> I believe the part in bold should be afterFind

2. Example repos

They point to the V1 sample app. There seems to be a V2 branch, but it does not contain examples with the different routers.

3. Many classes in the same collection

var child = new Child();
child.type; // "Child"

var parent = new Parent();
child.type; // "Parent" <--- what?

=>

var child = new Child();
child.type; // "Child"

var parent = new Parent();
parent.type; // "Parent" <--- probably meant this

4. Secured property

import { Class } from 'meteor/jagi:astronomy';

const User = Class.create({
  name: 'User',
  /* ... */
  // Turn off security for insert, update and remove operations.
  secured: false
});

As you can see, we switched on security for every operation on this class. Now let’s see how to secure only certain operations.

=> Didn't we just do the opposite?

Same section, later on:

In the example above, we turned off security only for insert and update operations.

Now let’s see what will happen if you try insert something with secured inserts turned on.

So first we turn off security for inserts, then we'll see what happens if we try to insert with secured inserts turned on. This is somewhat confusing?

Improvement ideas

1. Clarification on the small section about saving in the beginning of docs

Now, you don’t have to write Meteor methods in client and server to save a document. You can just write doc.save() on the client and the corresponding doc.save() method will be called on the server.

Then later, in the section titled: Storing documents - Server only call

By default, when you execute the save() method on the client, it will perform the save operation in both environments client and server. Thanks to that you don’t have to create Meteor methods, however it’s highly recommended for security reason. But more about that in the Security section.

  • This is confusing and could be hugely improved.
  • Am I supposed to not use methods or to use methods?
  • Does Astronomy use methods anyway, either way?
  • Does it involve allow/deny-rules to be used or set?
  • Is it against the recommended practice by MDG?
  • Should I actually not use the doc.save()-function on client, even though 80% of the documentation does?
  • What's the package authors recommended best practice using Astronomy?

I found some answers in the end of the document, in the Security-section, but this issue could be elaborated earlier and more profoundly, since it's a major design decision.

In a nutshell, Does Astronomy somehow magically make this allow-deny & save from client -approach viable again, even after MDG officially declared it as bad practice?

2. Mapping

How is this different from a transient field? Am I not returning some value based on another field and some random logic, just like with transient fields? Does it actually copy the mapped field phoneNumber to the new field phone in the database? I'm puzzled.

3. Getting values

In this part user.get(['firstName', 'lastName']); what is the actual return value of getting multiple fields?

4. Getting raw values

The raw() method is responsible for getting a plain value from a nested field. This means that even if a given field is defined as a nested Astronomy class, it will return a plain JavaScript object instead.

Can I no longer call document.raw() to get the raw object, like I could in V1? Is it only available for nested fields in V2?

5. Accessing document

So, just to clarify the example scenario with User and Address: in the User's beforeSave-handler, are e.target and e.currentTarget the same document?


DISCLAIMER: Sorry if I just misunderstood something, I'm just trying to improve the docs so others won't need to scratch their heads as much as I did :)

Thanks for the awesome package, I've been using Astronomy V1 for about six months and it's been a real gem.

lukejagodzinski commented 8 years ago
  1. Yes it should be afterFind
  2. Yes link is wrong but I'm not going to introduce example for IronRouter as it quite simple to do if anyone want to use it. However IronRouter seems to be replaced by FlowRouter by most developers.
  3. Yes it's typo
  4. Maybe it's because of changes I've introduced in 2.1 where security is turned ON by default. So probably I haven't changed all the descriptions.

Feel free to file a PR. I will merge it.

Improvement ideas

  1. You don't have to use meteor methods as long as you secure your classes properly. So as MDG said it's more reasonable to secure application through methods than allow/deny rules (events in Astronomy). Everything depends on your development skills. It's easier to secure application through Meteor methods. If you feel like it's not properly describe you can propose better description.
  2. Mapping is just a way of resolving some database field name to different field name. There is nothing complex here.
  3. It will return object:
{
  firstName: /* firstNameValue */,
  lastName: /* lastNameValue */
}
  1. You can use raw() to get raw document.
  2. Yes in the User's beforeSave event both currentTarget and target are the same. The only difference is for the nested fields.

Feel free to propose some changes. However, please put each change in different PR, so it will be to review and merge only if I need some of them. Thanks for noticing these errors.

arggh commented 8 years ago

I made a pull request with the fixes for now: https://github.com/jagi/meteor-astronomy/pull/490

lukejagodzinski commented 8 years ago

OK thanks! I will review it today and merge