naja-js / naja

Modern AJAX library for Nette Framework
https://naja.js.org
MIT License
108 stars 15 forks source link

Default application/json for manual requests #311

Closed dakur closed 3 years ago

dakur commented 3 years ago

Feature Request

I'm struggling with this all the time again and again. When I call naja.makeRequest() on my own, I always have to debug what's wrong and after some time I end up with this header missing.

Can't be JSON request&response expectation set by default? It is the most common use-case in my opinion.

Suggestion: add this to defaultOptions

fetch: {
    headers: {
        'Accept': 'application/json',
        'Content-Type': 'application/json',
    },
}
jiripudil commented 3 years ago

Accept: application/json, sure, why not, Naja expects JSON anyway and breaks if anything else is returned. I'd say it should even be hardcoded the same way that X-Requested-With is. PRs welcome 😉

Content-Type is a more delicate matter though.

It is the most common use-case in my opinion.

I'm afraid I can't base this decision on opinions and even less on a single one. From where I see it, the most common use case is naja.makeRequest('POST', '/url', { some: 'data' }) which is sent as application/x-www-form-urlencoded and is therefore easily processable server-side because Nette parses it into an array of post data.

Processing a JSON request body in a Nette application takes some extra effort, which makes me think it's not so common to send JSON data through Naja after all. But that's still just a guess too.

Maybe Naja could try to detect JSON values, but it's close to impossible to reliably tell what a string contains. Even the Fetch spec dictates to treat strings as text/plain unless specified otherwise. Would it be safe enough to assume data to be JSON if it's a string and JSON.parseing it doesn't throw an error? 🤷

dakur commented 3 years ago

Yeah, you're probably right, the {some: 'data'} is most common use-case. I forgot it can be done this way. 😁 That's because… The problem lies elsewhere. When I do manual request with an object literal:

const data = {some: 'data'};
naja.makeRequest('POST', '/url', data);

it works OK and is probably what everyone wants to do. But when you swap object for array which I would do when I want to send… well a list of whatever, it breaks:

const data = [{some: 'data'}, {some: 'data2'}, {some: 'data3'}];
// request body
[object Object],[object Object],[object Object] // ouch ❌

So what would I do? Let's say I know nothing about application/x-www-form-urlencoded content type (or even that it is used by Naja internally), how it relates to {} vs. [] or to HTTP protocol. I just expect it to work and it "does not work".

After some time of investigating/trying/thinking, I would go for JSON.stringify(). But it has to be sent with application/json header (which is likely to be forgotten – here we are) and decoded manually on the server.

const data = [{some: 'data'}, {some: 'data2'}, {some: 'data3'}];
naja.makeRequest('POST', '/url',
    JSON.stringify(data),
    {fetch: {headers: {'Content-Type': 'application/json'}}},
);
$this->request->getPost(); // empty array ❌
Json::decode($this->getHttpRequest()->getRawBody()); // expected data ✔

The second option is to wrap whole array into another object literal with some static key, which looks like magic a bit. On the other side, it works with Nette post data parsing.

const data = [{some: 'data'}, {some: 'data2'}, {some: 'data3'}];
const wrappedData = {whatever: data};
naja.makeRequest('POST', '/url', wrappedData);
$payload = $this->request->getPost();
$payload['whatever'] // expected data ✔

Well, until… you use non-POST method. As far as I understood from all of the discussions about HTTP and REST APIs, PUT is more semantical for CREATE actions. Nette parses post data from $_POST superglobal variable (which make sense) and thus data are not parsed on PUT. After all, it is easier to use JSON then to avoid HTTP layer specifics completely 🙃 (and it preserves data types as a bonus). But that introduces the aforementioned "forgotten content type" problem.

So… Yes, I agree that adding default application/json does not make sense. I'm still not sure what is the good approach and maybe there is no holy grail at all. But two things come into my head:

There was also similar topic on the forum (link) eventhough the problem was probably slightly different. Maybe some more knowledge about if this confuses/affects more people would be good first.

Or if you think the problem sketched above does not exist, feel free to close this issue. I don't want to take your time on hypothetical academic problems. 🙂

dakur commented 3 years ago

Just found this commit accidentally: https://github.com/wavevision/dependent-selectbox/commit/e145920fd7d18475cb6842df39eb50edc3002ae8#diff-684bcc3dc4c4256c56e68c20855c82ed6afc7cda0e44b5367846311b16fe6327

jiripudil commented 3 years ago

But when you swap object for array which I would do when I want to send… well a list of whatever, it breaks:

Good point. It seems to me like an edge case, but I don't see why it shouldn't be supported, since Nette parses 0=42&1=43 into a list as expected. Again, PRs welcome, I believe all it takes is an isArray condition here, plus tests.

Let's say I know nothing about application/x-www-form-urlencoded content type (or even that it is used by Naja internally), how it relates to {} vs. [] or to HTTP protocol.

Let's not say that. You can't work in any specialist position, and not know the tools you work with and the foundations you build upon. I'm not saying you should be able to cite RFCs by heart, but as a web developer, you should have some understanding of how HTTP works and how it transfers data.

That being said, I agree Naja should do a better job at explaining what kinds of transformations it does to different types of data. Which brings me to:

brief explanation of this stuff in docs by demonstrating when to use each solution on examples

Yes, that makes a lot of sense. I think it has been on my mind for #211, it was just missing from the roadmap there, apparently. I've added it to the issue.

Well, until… you use non-POST method. As far as I understood from all of the discussions about HTTP and REST APIs

Naja is not meant for REST APIs. Altogether, there have been numerous discussions and issues regarding various object/array transformations (#174, #175, #202, #253), and not a single one about JSON, until now. That makes me think it's not a common use case and I don't think it's worth implementing any special treatment at this moment, apart from the aforementioned example in the docs.

I understand it's easy to forget to add the header, but I see no reliable way of adding it automatically in Naja. And now that I think of it – what kind of issues do you run into? I mean, if the explicit header is missing (and the browser sends text/plain), maybe the webserver doesn't know it's JSON, but you as a developer do, and Json::decode($this->getHttpRequest()->getRawBody()) should still return the same result, regardless of the Content-Type header 🤔

dakur commented 3 years ago

And now that I think of it – what kind of issues do you run into? I mean, if the explicit header is missing (and the browser sends text/plain), maybe the webserver doesn't know it's JSON, but you as a developer do, and Json::decode($this->getHttpRequest()->getRawBody()) should still return the same result, regardless of the Content-Type header 🤔

Well, initially I thought that the missing header was reason why async requests failed sometimes (means turned first 7 chars into null chars in the JSON payload) in our app (some Apache's whim), but now it looks like an issue with our proxy server instead.

dakur commented 3 years ago

Fixing [] (#315) and adding use-cases into docs (#211) resolves this I think. Can I close this now?

jiripudil commented 3 years ago

Okay, sure, closing this.