mikehostetler / amplify

AmplifyJS
http://amplifyjs.com
GNU General Public License v2.0
1.45k stars 143 forks source link

Readme updates on amplify core. #47

Closed ifandelse closed 12 years ago

ifandelse commented 12 years ago

Additional usage examples in readme. These additions were prompted by https://github.com/appendto/amplify/issues/46. New examples explain subscribing to multiple topics in one subscribe call, and how to pass the "topic" as a param to the subscription callback. Also - the addition of the gitignore file at the root is to keep WebStorm from adding project metadata to the repo.

scottgonzalez commented 12 years ago

In general I don't like the idea of including how to build new features in the main docs. I'd much rather us write a full tutorial that walks through the logic and concludes with a full features solution. I'll leave this decision to the rest of the team. If we want to keep the topic-passing example in the readme, I'm ok with landing it after the code is updated.

dcneiner commented 12 years ago

I think in general I agree with you @scottgonzalez. The only thing that is a little different here, is that this is actually a common problem caused by the ability to subscribe multiple topics at once. Its a great feature (multiple subscriptions), but without the ability to see which topic fired, its somewhat limiting.

I think adding documentation (vs a tutorial) is a nice compromise if we can not add the actual feature. It addresses the need, but keeps the library simple.

scottgonzalez commented 12 years ago

We can link to the tutorial directly from the docs if your concern is purely about discoverability.

ifandelse commented 12 years ago

I wasn't aware that there was a separate place (another repo?) to write tutorials. I've put effort into explaining the multiple subscriptions because I've run into multiple developers using amplify that aren't aware of it. Second, I think the "how to get the topic from within the callback" is question that is going to come up over and over again - and considering that one developer expressed that he wouldn't use amplify unless this were possible, having information on how to do this somewhere accessible is wise, IMO. I will update the code formatting, etc. in case we end up wanting to merge this.

scottgonzalez commented 12 years ago

We would put it on the .delegate() blog and link to the article from the readme.

ifandelse commented 12 years ago

These changes should be sufficient. I vote we either just go ahead and kill this thing dead, or merge it.

scottgonzalez commented 12 years ago

Why did you switch to two functions? You're duplicating work on every subscription now.

ifandelse commented 12 years ago

@scottgonzalez the only duplication i see is the arg parsing that was happening in the wrapper function, as opposed to the subscribe call. Small change, IMO. I moved it.

ifandelse commented 12 years ago

Final ping - are we merging this or not?