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

design new dsl #88

Open
kristofsajdak opened this issue May 28, 2015 · 7 comments
Open

design new dsl #88

kristofsajdak opened this issue May 28, 2015 · 7 comments

Comments

@kristofsajdak
Copy link

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
Copy link
Author

This is a bit reactive to @mavdi PR, but I kind of agree after thinking about it a bit more
#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
Copy link

dclucas commented May 28, 2015

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
Copy link

mavdi commented May 28, 2015

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
Copy link
Author

@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
Copy link
Author

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

@ssebro
Copy link

ssebro commented May 29, 2015

@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
Copy link

ssebro commented Jun 1, 2015

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants