agco / harvesterjs

Create JSONAPI-compliant APIs over a Node.js + MongoDB stack in an easy, boilerplate-free manner
http://agco.github.io/harvesterjs/
MIT License
68 stars 13 forks source link

design new dsl #88

Open kristofsajdak opened 9 years ago

kristofsajdak commented 9 years ago

The new dsl design has been discussed off record within the team at various occasions, creating an issue here so we can track further discussion

This is on the table right now : https://gist.github.com/kristofsajdak/73aadf4393c5526f3705/1b35d1b1debfc6dc707193421753f6d9d9b3c7ac

kristofsajdak commented 9 years ago

This is a bit reactive to @mavdi PR, but I kind of agree after thinking about it a bit more https://github.com/agco/harvesterjs/pull/87

We are probably optimising for the edge case scenario, drafted a proposal just now which could make things a bit less verbose : https://gist.github.com/kristofsajdak/73aadf4393c5526f3705/3099668f707c829fbcb6f31992e7990e5f4d83c9

This gets rid of the need to explicit declare each route in isolation, it's now only required when we you want to override defaults for authorisation, swagger, validation or when you want to add custom logic.

dclucas commented 9 years ago

Some comments/questions:

  1. I'm guessing most team won't agree, but I prefer to keep APIs clean from their dependencies. So I would rather not use Joi.string() or .swagger( { ... }), but instead use something like A.string() and document(...);
  2. About the current form of the swagger function: my suggestion is that it receives the autogenerated documentation as an argument. This would allow the developer to cherry pick the documentation he or she wants to override. Also, a question: I prefer returning the docs, but this forces people to always return in that method. An alternative is to embrace a side-effect here and allow the developer to manipulate the input argument. Still vote for return'ing, though;
  3. Can/should before and after accept function chaining, as in .delete().before(func1, func2)?
  4. Shouldn't we also provide a response object for these closures, as in .before(function(req, res) {...} )?
  5. Should it be possible to compose across before, handling (where harvester.js does its magic) and after functions (before being able to do some processing and make it available for later steps, after being able to transform the current response somehow)?
mavdi commented 9 years ago

I agree. Declaring each method seems to be excessive.

I like your proposed dsl @kristofsajdak but as I mentioned in our call it would be very tricky to implement such a DSL and have a functioning v1. IMO we should do a clean break, implement the dsl, tests things slightly better and then go and move the projects that use harvester to v2.

@dclucas I agree. Hapi used to do Hapi.types which just mapped to Joi. So maybe we can indeed use Harvester.types and .docs

On your 3rd point, could be nice but maybe down the line. Lets strip features to the minimum for now.

kristofsajdak commented 9 years ago

@dclucas @mavdi agreed on 1. updated the gist accordingly https://gist.github.com/kristofsajdak/73aadf4393c5526f3705/aea8ed3469d1eaa19792b3cd5050bf8ade8596a6

@dclucas 2. no reason why we can't have both

@ssebro and myself have discussed 3. 5. , but due to lack of real use case we applied a bit of YAGNI, the current design is a change increment we can commit to as a team

kristofsajdak commented 9 years ago

@mavdi let's discuss with @ssebro whether we need to keep it backwards compatible

ssebro commented 9 years ago

@kristofsajdak @dclucas @mavdi

I don't have much to add here - I agree with the current state of the discussion.

wrt backwards compatibility - I never imagined that today's clients would be able to keep using harvester v2.0 without any changes.

The backwards compatibility I wanted was strictly for the object stored in _schema, because it powers several things outside harvester.

ssebro commented 9 years ago

@kristofsajdak @dclucas @mavdi

Chatted with @mavdi about how the dsl might be implementable in a way that's mostly backwards compatible, so full backwards compatibility may actually be on the table...