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

feat: add new method on Request class #35

Merged
merged 7 commits into from
Nov 24, 2020
Merged

feat: add new method on Request class #35

merged 7 commits into from
Nov 24, 2020

Conversation

shinspiegel
Copy link
Contributor

This is a starting point for the discussion to this method.

In this way even if the JSON information is broken or it is a invalid will return null. What do you think?

I can reduce the code the code to a single line or anything like that, but I think it's cleaner to see the intend in this way.

What I can to to improve it?

@ije
Copy link
Member

ije commented Nov 17, 2020

hey @shinspiegel , i'm thinking maybe it should be decodeBody or parseBody, so we can decode the body for different content types, like form-data.

interface FormDataBody {
  get(key:string): string
  getFile(key:string): FormFile
}

interface FormFile {
  filename: string
  contentType: string
  content: Uint8Array
}

interface Request {
  ...
  decodeBody(type: 'text'): Promise<string>
  decodeBody(type: 'json'): Promise<any>
  decodeBody(type: 'form-data'): Promise<FormDataBody>
}

what do you think?

@ije
Copy link
Member

ije commented Nov 17, 2020

or useBody?

@shadowtime2000
Copy link
Member

@ije I think it would be best to only use the use- prefix for React hooks to make it easier for people to distinguish functions and hooks.

@ije
Copy link
Member

ije commented Nov 17, 2020

@shadowtime2000 yes you are right!

@shinspiegel
Copy link
Contributor Author

Hi,

I was trying to overload the method, but the intellisense keeps getting a error.

This is the right way of doing this in typescript?

async decodeBody(type: "text" | "json" | "form-data"): Promise<string | any | FormDataBody> {
...
}

Or this is the right way?

async decodeBody(type: "text"): Promise<string>
async decodeBody(type: "json"): Promise<any>
async decodeBody(type: "form-data"): Promise<FormDataBody> {
...
}

Or there is other way of doing?
I did try to find the right way in the docs but I totally lost my way on this subject in TS docs.

I was reading the Deno.Land docs to parse the form data, but I didn't find how to properly use it. I'll keep digging tomorrow.

Ps.: decodeBody is a totally better name than jsonBody 😄

@ije
Copy link
Member

ije commented Nov 18, 2020

@shinspiegel well done! thank you!

  • to overload the method you can do like below code:

    class Request {
        ...
        decodeBody(type: 'text'): Promise<string>
        decodeBody(type: 'json'): Promise<any>
        decodeBody(type: 'form-data'): Promise<FormDataBody>
        async decodeBody(type: string): Promise<any> {
            // decode
        }
        ...
    }
  • to parse multipart/form-data body please ref https://deno.land/x/oak@v6.3.2/multipart.ts

@ije ije linked an issue Nov 19, 2020 that may be closed by this pull request
@shinspiegel
Copy link
Contributor Author

Hi, sorry for late response (couple of busy days)

I did try to use the TS the way show below, and for some reason the Deno compiler just doesn't get it. I'm doing something wrong? (I'm new to TS, trying to catch up)

class Request {
    ...
    decodeBody(type: 'text'): Promise<string>
    decodeBody(type: 'json'): Promise<any>
    decodeBody(type: 'form-data'): Promise<FormDataBody>
    async decodeBody(type: string): Promise<any> {
        // decode
    }
    ...
}

The VSCode still gives me error on this one.

Can I use the code from other source like multipart/form-data body please ref deno.land/x/oak@v6.3.2/multipart.ts?

Or should I create a complete separated file with the complete (and a little bit changed) version of theirs?

@ije
Copy link
Member

ije commented Nov 22, 2020

hi @shinspiegel thanks for your great work, it's ok with oak, maybe we can implement the multipart module in the future.

Copy link
Member

@ije ije left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

api.ts Outdated
@@ -84,6 +85,51 @@ export class Request extends ServerRequest implements APIRequest {
await this.send(JSON.stringify(data, replacer, space), 'application/json; charset=utf-8')
}

async decodeBody(type: "text"): Promise<string>
async decodeBody(type: "json"): Promise<any>
async decodeBody(type: "form-data"): Promise<FormDataBody> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here should be:

    decodeBody(type: 'text'): Promise<string>
    decodeBody(type: 'json'): Promise<any>
    decodeBody(type: 'form-data'): Promise< FormDataBody >
    async decodeBody(type: string): Promise<any> {
       if (type === "text") {
          ...
       }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now the Deno compiler and the VSCode intellisense aren't angry with me 😄

@ije ije mentioned this pull request Nov 22, 2020
30 tasks
@shinspiegel
Copy link
Contributor Author

I guess it's done...
Or should I change something?

@ije
Copy link
Member

ije commented Nov 24, 2020

@shinspiegel it looks amazing, thanks

@ije ije merged commit 9596ac9 into alephjs:master Nov 24, 2020
@alfredosalzillo
Copy link
Contributor

decodeBody(type: 'json'): Promise<any> could be

decodeBody<T = any>(type: 'json'): Promise<T>

so no need for a cast to have the right type.

@alfredosalzillo
Copy link
Contributor

@shinspiegel

In this way even if the JSON information is broken or it is a invalid will return null. What do you think?

I think if the JSON is invalid should not return null but throw an exception because it's to the developer using it handle this case.

@shinspiegel
Copy link
Contributor Author

@alfredosalzillo

I think if the JSON is invalid should not return null but throw an exception because it's to the developer using it handle this case.

A unhandle exception would broke the application, stopping from working, right?

@alfredosalzillo

decodeBody<T = any>(type: 'json'): Promise<T>

This looks like a great idea!
I'll make it soon!

@alfredosalzillo
Copy link
Contributor

A unhandle exception would broke the application, stopping from working, right?

Yes, but how I know if the response body is null or if it's an invalid JSON, without exception?
For example, the json in the response object of the fetch doesn't handle the exception.

@ije
Copy link
Member

ije commented Nov 24, 2020

@shinspiegel sorry, you can not validate the types by

decodeBody<T = any>(type: 'json'): Promise<T>

you should create your own validate function:

type User = {
  id: number
  name: string
}

function isUser(v: any): v is User {
  return typeof v === 'object' && v !== null && typeof v.id === 'number' && typeof v.name === 'string' 
}

const body = await req.decodeBody('json')
if (isUser(body)) {
  // body type is User
} else {
  req.status(400).send("bad request")
}

@shinspiegel
Copy link
Contributor Author

@ije what about the thowing exception on the JSON parse, should I improve it?
In this case I should make another pull request with this change?

@ije
Copy link
Member

ije commented Nov 25, 2020

@shinspiegel i think we should remove the try catch, let user catch errors themselves:

try {
  const data = await req.decodeBody('json')
  req.status(200).send("hello " + data.name)
} catch(e) {
  req.status(400).send("bad request")
}

@ije
Copy link
Member

ije commented Nov 25, 2020

@shinspiegel do you have time to implement a multipart mod as alephjs part? i saw the oak/multipart imported some deps alephjs never used. or i can add it later, thanks for your great work!

@shinspiegel
Copy link
Contributor Author

@ije
@shinspiegel do you have time to implement a multipart mod as alephjs part? i saw the oak/multipart imported some deps alephjs never used. or i can add it later, thanks for your great work!

Sure I'll make a new branch and start my next free night. That will be a excellent opportunity to learn a thing or two.

@ije
@shinspiegel i think we should remove the try catch, let user catch errors themselves:

try {
  const data = await req.decodeBody('json')
  req.status(200).send("hello " + data.name)
} catch(e) {
  req.status(400).send("bad request")
}

I'll change the try/catch block on the same branch/issue.

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.

A way to get a JSON from a request body
4 participants