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

Implement and recommend a DRYer approach #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement and recommend a DRYer approach #1

wants to merge 1 commit into from

Conversation

vemv
Copy link

@vemv vemv commented Jun 2, 2018

Hi!

Thank you for your work. I am using this library, but in a wrapped manner because I found it to be verbose. I'd love to be able to use the library directly instead, sharing my recipe with others.

So this PR intends to add an alternative higher-level API, and document it as the recommended one.

What issues did I find with the original approach?

  • using-component, as intended, goes in a per-route basis, which seems unnecessary. Why not give the whole system map to every request of every route? Else it's more code to write and more coupling to introduce (I have to remember to both add this using-component clause in the routing part, and to fetch it later with use-component. If I change one part -> I have to update the other)

  • use-component is a glorified get. If component keywords were directly associated to the request map, then I could leverage destructuring:

(defn my-handler [{:keys [db sms worker] :as request}] ...)

If keyword clashes were a concern, developers could opt to use ns-qualified keywords instead of plain :db etc. I'd actually recommend that, but it should be each developer's choice. using-component goes in the opposite direction, forcing safety at the cost of verbosity.

Finally, the app/:app thing in the README seems a good practice but also an unnecesary level of indirection in the context of learning a library. My changes respect :app, but I would recommend to remove it, so :pedestal depends directly on :db (via components-to-inject).

I hope this sounds like a reasonable changeset.

Cheers - Victor

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

Successfully merging this pull request may close these issues.

1 participant