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

Allow json to be called multiple times #129

Merged
merged 7 commits into from
Jan 25, 2017
Merged

Allow json to be called multiple times #129

merged 7 commits into from
Jan 25, 2017

Conversation

onbjerg
Copy link
Contributor

@onbjerg onbjerg commented Jan 22, 2017

Consider a wrapper function that calls the json function, while wrapping another function that calls the json function as well, e.g.

const { json } = require('micro')

const wrapper = function (fn) {
  return async function (req, res) {
    console.log('wrapper', await json(req))
    return await fn(req, res)
  }
}

module.exports = wrapper(async function (req, res) {
  console.log('main', await json(req))
  return 'stuff'
})

The call to json by the wrapper would block the next call indefinitely, since the stream from req has already been read.

This PR solves that.

@timneutkens
Copy link
Member

@rauchg I monkey patched this in micro-dev. What would you prefer? If it's in micro core, then please do merge 😄

@onbjerg
Copy link
Contributor Author

onbjerg commented Jan 25, 2017

@timneutkens It shouldn't be a monkeypatch since it's true for all wrapper functions that need to use json.

@@ -45,7 +45,9 @@ async function json(req, {limit = '1mb'} = {}) {
const type = req.headers['content-type']
const length = req.headers['content-length']
const encoding = typer.parse(type).parameters.charset
const str = await getRawBody(req, {limit, length, encoding})

req.rawBody = req.rawBody || getRawBody(req, {limit, length, encoding})
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use a WeakMap so that we don't touch req?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where would you store the WeakMap? Just outside the function?

Copy link
Member

Choose a reason for hiding this comment

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

Yep

@rauchg
Copy link
Member

rauchg commented Jan 25, 2017

This is a pretty big semantic change. We would have to document it.

@onbjerg
Copy link
Contributor Author

onbjerg commented Jan 25, 2017

Sure, I'll document it, too.

@rauchg
Copy link
Member

rauchg commented Jan 25, 2017

Made a little clarification in the code. Let me know what you think

@onbjerg
Copy link
Contributor Author

onbjerg commented Jan 25, 2017

Yeah, I think it makes sense to add a comment to clarify what's happening. I'm fixing the test.

@onbjerg
Copy link
Contributor Author

onbjerg commented Jan 25, 2017

I documented the behavior. Any thoughts?

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@timneutkens timneutkens merged commit db823f0 into vercel:master Jan 25, 2017
@timneutkens
Copy link
Member

Thing I'm wondering, should we add a rawBody or body function now. To expose the cache to external modules. We've been holding off on this since it would be possible without (https://github.com/timneutkens/raw-text).

@onbjerg
Copy link
Contributor Author

onbjerg commented Jan 25, 2017

@timneutkens You can read the raw body with other modules from the request, since bodies can be formatted in different ways and thus are parsed differently. I don't think having a body function would serve any purpose

But maybe I'm wrong

@timneutkens
Copy link
Member

Yeah it's just the case of calling json on it and then urlencoded on it for example, where it would fail because the body was already parsed. It's an edge case though, I don't mind not supporting it.

@onbjerg
Copy link
Contributor Author

onbjerg commented Jan 25, 2017

@timneutkens Is it even possible to have multiple bodies in the same request? I don't think I'm following

@onbjerg onbjerg deleted the patch-1 branch January 25, 2017 19:21
@timneutkens
Copy link
Member

I was explaining a pretty far fetched use case where you'd want to do something with the request body if json parsing fails. But nvm that, it's not important 👍

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.

3 participants