search

meteor / todos

The example app "Todos", written following the Meteor Guide
Other
535 stars 367 forks source link

Extract current local factory package #193

Closed hwillson closed 7 years ago

hwillson commented 7 years ago

This is a related to the discussion in meteor/docs#96.

The short term plan is to extract the local factory package and publish it as a Troposhpere/Atmosphere based package, then update both the todos app and Guide to refer to this new package.

The longer term plan (if someone has the time) is to potentially look into merging the todos app factory package with the dburles:factory package, and coming up with a new factory package offering. See meteor/todos#138.

dburles commented 7 years ago

I'm not sure updating the guide to reference the todo's package would be ideal as I believe the re-write was done for a specific purpose based on how the tests were written in the todos app. Not to mention the wide adoption of dburles:factory. Plus I am slightly biased here of course.

I think I need to make some time to figure out a plan with @tmeasday to move forward.

We need to answer these questions:

  1. Recall the reason as to why it was rewritten.
  2. Which features are missing from the rewritten version when compared to dburles:factory.
  3. Are the API's incompatible?
  4. Could we update dburles:factory to work like todos/factory (does the new tree method do this?)
  5. Also considered was 'version 2.0' of dburles:factory simply dropping in todos/factory, however I don't think that's really ideal as dburles:factory is more feature rich.

Phew. :)

dburles commented 7 years ago

Hmm, just thinking about it a little, maybe the simplest way to answer most of those questions would be to drop dburles:factory into the todos app and try to make it work. I'll make some time to do that.

hwillson commented 7 years ago

@dburles That would be awesome! Let me know if I can help with anything. Thanks!

tmeasday commented 7 years ago

Recall the reason as to why it was rewritten

Basically I wanted to build a version that worked when you wanted to build associated data client side without saving to a database. That's why the new version did a lot of work to build a "dataset" for you.

I guess that same idea is way more natural in the GraphQL world (and pretty trivial with graphql-anywhere, I guess?) so I don't think it's worth pursuing any further in livedata-land.

Also, using stub-collections is another pretty good way (if slightly hacky) to achieve the same result.

dburles commented 7 years ago

Basically I wanted to build a version that worked when you wanted to build associated data client side without saving to a database. That's why the new version did a lot of work to build a "dataset" for you.

I think the tree method should be able to handle that, though the result shape does differ https://github.com/versolearning/meteor-factory#tree

I guess that same idea is way more natural in the GraphQL world (and pretty trivial with graphql-anywhere, I guess?) so I don't think it's worth pursuing any further in livedata-land.

I'm inclined to agree, so I think the path of least resistance might be to simply use dburles:factory in the todos app. I've switched it out in this PR https://github.com/meteor/todos/pull/202

tmeasday commented 7 years ago

WFM!

hwillson commented 7 years ago

Excellent @dburles! Merged and closing this off. Thanks very much!