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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate params and http body to avoid confusion #64

Open
olaven opened this issue Apr 14, 2020 · 1 comment 路 May be fixed by #67
Open

Separate params and http body to avoid confusion #64

olaven opened this issue Apr 14, 2020 · 1 comment 路 May be fixed by #67

Comments

@olaven
Copy link
Contributor

olaven commented Apr 14, 2020

Hi! Again: thanks for this project 馃槃

I am wondering if it would make more sense to separate the HTTP response body from params, in Context. This makes the code more explicit and readable, in my opinion. For example:

app(post("/posts/:id/comments", ({ params, body }) => {

    console.log(`comment on post: ${params.id}`); 
    console.log(`comment data: `, body); 
    return [201, "Created comment"];
}))

Furthermore, this may cause ambiguity as to what data belongs to URL params and what data belongs to the HTTP body. Consider this (somewhat contrived) example:

interface Comment {
  post_id: string, 
  id: string, 
  text: string, 
};

app(
  // Todays solution: 
  post("/posts/:id/comments", ({ params }) => {

    //`params.id` -> ID of post or ID of comment? 
    const comment = { post_id: params.id, id: params.id, text: params.text}
    return [201, "Created comment"];
  }),
  // Proposed solution:  
  post("/posts/:id/comments", ({ params, body }) => {

    // No ambiguity between `params.id` and `body.id`
    const comment = body 
    return [201, "Created comment"];
  })
);

I implemented a solution here. If you're interested, I'll submit a PR 馃槃
However, I believe this breaks with Sinatra, so it's completely reasonable to dissagree.

What do you think?

EDIT: typo and added link to implementation

@olaven olaven linked a pull request Apr 18, 2020 that will close this issue
@olaven
Copy link
Contributor Author

olaven commented Apr 24, 2020

Are there any changes nececcary for the PR for this to get merged?
I would be happy to implement them! 馃槃

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 a pull request may close this issue.

1 participant