Skip to content
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

Test helper packages #119

Closed
hwillson opened this issue Apr 21, 2016 · 23 comments

Comments

@hwillson
Copy link
Member

commented Apr 21, 2016

Hi guys! I find the test helper packages (publication-collector, the modified factory package, and stub-collections) to be quite helpful. Are there any plans to publish these, or would you be open to PR's to help get them ready? Thanks!

@hwillson

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2016

Just to add (since I know you guys are busy) - if you prefer, I can volunteer to take ownership of them, clean them up a bit, publish, and support them. Unless of course you guys already have plans to do this. Let me know - thanks!

@msio

This comment has been minimized.

Copy link

commented Apr 21, 2016

Hi @hwillson did you make working publication-collector package because I got this error #120
Thanks

@hwillson

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2016

Hi @msio777 - commented in #120.

@tmeasday

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

@hwillson I would totally love you to take over them and publish them yourself. Some notes:

  1. publication-collector: This one works pretty well right now but I haven't tested it much outside of the todos app.

  2. factory: I'm not sure what the best thing to do with this one is. @timbotnik and I basically rewrote dburles:factory to use different style of object creation in order to allow creation of "datasets" without having to write to Mongo. The best thing would be to merge it into dburles:factory I think, but it's pretty non-trivial to do. I don't think there's much long term benefit in maintaining two packages with basically the same API.

    PS I'd also love to see a non-globals based API for that too. Like:

  import todosFactory from '/todos/factory.js';
  todosFactory.create();

  // rather than
  Factory.create('todos');
  1. stub-collections - I think this one is pretty good as is, but definitely would love someone to maintain it!
@tmeasday

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

@dburles

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

Hey @tmeasday I suppose we could release it as version 2.0 of the package (without backwards compatibility).

@hwillson

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

Great, thanks guys! @tmeasday - I'll start by breaking out publication-collector and stub-collections, adding a bit more info to the docs, and getting them published. I'll then take a look at the factory approach; @dburles a 2.0 version sounds like a great idea. I could clean things up a bit first to get rid of the globals, and we can then re-visit. We'll be in touch!

@msio

This comment has been minimized.

Copy link

commented Apr 22, 2016

@hwillson great I'm looking forward to it

@hwillson

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

Hi guys - sorry for the delayed follow up on this. I've broken a new version of the first package (stub-collections) out here. More coming soon. Thanks!

@dburles

This comment has been minimized.

Copy link
Contributor

commented May 3, 2016

Hey @hwillson @tmeasday I've gone ahead and setup a new branch for version 2 https://github.com/versolearning/meteor-factory/tree/two

@hwillson

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

Awesome @dburles, thanks!

@hwillson

This comment has been minimized.

Copy link
Member Author

commented May 20, 2016

Quick update based on #136 - @johanbrook is going to look into splitting out the PublicationCollector package (thanks @johanbrook!).

@johanbrook

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

Thanks for referencing here as well, @hwillson !

@johanbrook

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

Alright, extracted to GitHub repo and published to Atmosphere:

Added some basic Mocha tests and fixed README a bit.

@tmeasday

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

Looks great @johanbrook.

If you get a sec, send us a PR to switch out the dependency here and at at the guide!

@johanbrook

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

For sure :)

Question: did you guys have any plans on next steps with this package, feature wise? I read here and there that "it can be improved greatly". I'd be keen to listen.

@tmeasday

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

I can't recall exactly -- I'd have to play with it in an app again, but I can think of two main thrusts:

  1. I'm pretty sure I hacked it together (based on @stubailo's simple:rest code) in a short period of time, so I can't really vouch for the underlying code. I saw it as a proof-of-concept (although it seems to have stood up well so far; maybe a sign that Sashko's code was good!)
  2. Does it support publications changing reactively as you call methods/write to Mongo? That'd be the obvious place to take it I think.
@johanbrook

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

Gotcha, thanks! The code is very "clean and minimal", and it seems to do what it's supposed to do so far.

@dburles

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

@johanbrook Could be updated to ES6 :)

@johanbrook

This comment has been minimized.

Copy link
Contributor

commented May 22, 2016

@dburles You mean imports? The code pretty much uses ES2015 syntax already :)

@dburles

This comment has been minimized.

Copy link
Contributor

commented May 22, 2016

@johanbrook you're right, at a glance what stood out was that it could probably be expressed in the new class syntax!

@johanbrook

This comment has been minimized.

Copy link
Contributor

commented May 22, 2016

@dburles True true. If we'd like that, don't think it'll hurt in this case.

@hwillson

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2016

Both the publication-collector and stub-collections packages have been moved into their own separate packages (johanbrook:publication-collector and hwillson:stub-collections respectively). The custom factory package is going to stay as is (for now anyways - if anyone wants to revisit it, let me know). Closing!

@hwillson hwillson closed this Oct 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.