[FEATURE] http requests snippets

[fejes713]

Description

I've added basic CRUD calls. They are implemented using XMLHttpRequest web API.
In case someone missed the discussion on gitter earlier today this is the reason we are adding those snippets:

yeah it's the introductory way and it's easier to figure out, promises and fetch can be hard to grasp for newbies

I think I've done everything correctly. I would like someone to review changes and descriptions before we merge this. I had few problems with Git but everything should be fine.

What does your PR belong to?

• Website
• Snippets
• General / Things regarding the repository (like CI Integration)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Enhancement (non-breaking improvement of a snippet)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have checked that the changes are working properly
• I have checked that there isn't any PR doing the same
• I have read the CONTRIBUTING document.

[Chalarangelo]

On first sight, these seem completely fine. I will run a couple of tests tomorrow morning with live APIs that I have keys for, as well as a custom implementation I have set up locally and tell you if anything is broken (which should not be). Otherwise, we are ready to merge. Thanks a bunch for adding these in!

[skatcat31]

Why are we implementing these?

[fejes713]

@skatcat31 Everyone makes requests 💯 The new people in JS world might find Promises and fetch API too hard at the beginning and they just want to make CRUD request. They're going to search on how to do it and these snippets would help them since they are simple and easy to understand, well explained and mostly they just grab and go.

[skatcat31]

@fejes713 not disagreeing everyone will need to make remote requests at some point.

I do however want to point out that both libraries are confusing becasue they are trying to abstract the network layer and that promises have nothing to do with the complexity of learning network communications. If they can't understand promises that is a totally different issue.

After all I can implement the same footprint with fetch too:

const httpGET = (url, callback, err = console.error) =>
  fetch(url, { method: 'GET' }).then( (res,error) => error ? err(error) : callback(res))

and neither implementation actually fixes the problem of having to learn how to handle the response.

Your example makes an assumption, but truth is that it's still a request fulfillment object. To handle the response you would need to learn either library to know what the next step is which your code doesn't address and expects them to know.

[Chalarangelo]

@skatcat31 Handling the response is a bit of an issue, agreed. Providing a link to the MDN documentation could help some people that grab a snippet from here kickstart their development process and not get lost. It's either that, explaining in depth how to do so (which is way out of scope for 30 seconds) or not adding the snippets altogether.

I think providing links and adding an advanced tag to make sure people pay attention might be just about enough as an introduction. We can always add them as a test and, if feedback is negative, we should archive them in the long run. Opinions?

[skatcat31]

@Chalarangelo that's the same as implementing it with fetch and tell them to go read the fetch documentation. Either way our code hasn't actually solved anything. They still need to go read the documentation... This is a hard subject to approach due to the complex nature of network requests and how many infinite ways there are to implement it, most ending in full blown libraries due to response handling.

Hence why I'm for the third option since we can't even scratch the surface without going past the scope of the repo.

After all if they're misunderstanding promises we have solved nothing since now they're still confused about how to do network requests and have to read a library to advance, and still confused on how promises work.

[Chalarangelo]

I am putting on hold and adding opinions needed as I would like to see how the rest of the community feels in regards to these. I vote to keep them and see how this goes, but I need a few people's take on this to help us decide.

[iamsoorena]

I don't know why but I feel like implementing these are wrong. I think maybe It's related to that we may confuse people as to why they should use third party packages like axios or even native fucntions like fetch.

[fejes713]

Explanation

I was in the same boat a few months ago, I had to make a get request but I didn't know how to do it, back then the first thing that I saw when I googled it was XMLHttpRequest API. I didn't even try to understand it since it was working as soon as I copied it in my code.

Not all users that come to our website will have the same skill level, however, we should provide learning material for everyone. Personally, I wouldn't use XMLHttpRequest now, instead, I use fetch API, but 3 months ago when I had no clue what are the promises and how do they work, the standard plain JavaScript HttpRequest was more than enough for me. I just copied it, it worked and solved my problem.

As mentioned in every 3rd PR, we are making the collection of snippets and we're not trying to make a library, so it is perfectly fine for me to include both XMLHttpRequest and fetch API for our users. That said everyone will find what they need, the beginners would solve their big problems without touching promises or any other third party packages like axios.

[iamsoorena]

@fejes713 and I also feel bad cause XMLHttpRequests are scary :)

[atomiks]

@Chalarangelo 5+ params is generally discouraged as it makes it unwieldy to call. Usually you just use an object in that case

[Chalarangelo]

@fejes713 That would simplify things but what if a user sends data in a 'GET' request? Or would the API just discard the data anyways?

[Chalarangelo]

@atomiks I completely agree with you, it's just that using an options object is something I forget how to do at times, so I just left it that way. I am all for making the only necessary arguments the verb, url and callback and let the others be part of an options object or something like that.

[atomiks]

Promises and XMLHttpRequest. That is a really bad idea

axios is promise-based and uses XHR. There is nothing wrong with using a promise instead of a callback with a certain API

[Chalarangelo]

@atomiks Don't get me wrong, I prefer promises when I have the chance to use them, even though I am not very confident in my abilities to utilize them all the time. However, teaching two difficult things in one snippet will definitely confuse people.

[Chalarangelo]

After giving this some thought, I tried to simplify my above suggestion, based on the following:

• Setting a header should not cause any trouble in any case.
• GET requests can send data (and even if they aren't supposed to, the server-side API should know what to do or will discard them).

const httpRequest = (verb, url, callback, data = null, err = console.error) => {
  if(!['GET', 'PUT', 'POST', 'DELETE'].includes(verb.toUpperCase())) 
    throw new Error('Verb not supported!');
  const request = new XMLHttpRequest();
  request.open(verb.toUpperCase(), url, true);
  request.setRequestHeader('Content-type','application/json; charset=utf-8');
  request.send(data);
  request.onload = () => callback(request);
  request.onerror = () => err(request);
};

[fejes713]

That would simplify things but what if a user sends data in a 'GET' request

@Chalarangelo Well ... GET returns you some data so there is no need to send it. I believe everyone knows that much... Anyway, our snippets aren't bulletproof, in many snippets if you send the wrong type of data you would run into an error.

[Chalarangelo]

@fejes713 Apart from that, I think I have seen APIs that kinda want you to send data in a GET but I might remember something wrong there.

[fejes713]

@Chalarangelo What should we do then?
Do you want me to merge those 4 into 1?

[Chalarangelo]

We need to vote. I believe there's value in exposing developers to both fetch and XMLHttpRequest so I vote we make one snippet for each (merge these 4 into one and implement a similar one for fetch). However, so far we have gathered some mixed opinions, so I'd like to hear back from everyone who can and see where we stand on this, before proceeding.

@fejes713 @atomiks @iamsoorena @skatcat31 @kingdavidmartins cast your votes please (everyone else is also welcome, I am just tagging some people to notify them).

[skatcat31]

I vote to close due to complexity of implementation. We would almost be guaranteed to need to deference an object in the arguments to keep from involving an anti pattern( current implementation has the anti pattern in it's footprint )

If we do do this we should resolve the anti pattern by changing the footprint to ( String verb, String uri, Object options, Function success, Function error )

However at that point this becomes a fairly long block of code requiring checking against the keys in the object and definition to make sure requests are correctly setup.

If we do want to do a simple GET or POST example(s) I am more open to that and leaving the DELETE, PUT, OPTIONS, and HEADER verbs out of the repo in either XMLHTTPRequest OR Fetch

[Chalarangelo]

@skatcat31 Actually GET and POST should suffice, the rest are pretty similar anyways.

[skatcat31]

To be fair I am electing for SIMPLE GET and POST where the expected return is a string(html or plain or JSON for them to parse) which is a very simple to impliment snippet but will error on any other return.

I do still think XMLHTTPRequest should be avoided but that's only my personal opinion. fetch after learning it is much more consistent and easier.

I will admit many people have a hard time using promises, but I think that's because they aren't used to async/event based programming and many of those same people don't understand the full ramifications of callbacks or this inheritance either...

[Chalarangelo]

Is there any danger of XMLHttpRequest being deprecated or anything? I mean fetch is essentially the same but superior, so if we have reason to only use fetch, we could do that, even if promises seem more intimidating at first. Otherwise, I'm ok with both.

Simple GET and POST is the only kind of thing we can implement anyways. Anything else will need us to make assumptions about APIs or be API-speciic (kinda). Most APIs I have worked with return strings, JSON or XML, so these should suffice for a user getting to know http requests.

[mariastervic]

If only get and post are included as requests, there can either be two snippets or one that defaults to get. My opinion is that http requests are useful learning resource and they could help new developers.

The footprint is long however, so something like verb, url, callback, data = null, err = console.error makes it easier to work with, as you need only specify the type of request, url and the callback.

As far as the response is concerned, responseText should be passed to the callback, so that it's always a string value. That way it's easier to explain and follows the idea of a SIMPLE request as suggested above.

[Chalarangelo]

I think that just passing the responseText to the callback seems like a good introduction for developers and that it could make the whole thing less troublesome.

My suggestion is that we update all snippets we currently have in this PR to do that, archive PUT and DELETE and only keep GET and POST in the main website/README, adding a proper explanation, links and real-life-like examples (jsonplaceholder can be a decent way to show off). That way we will introduce developers to a fairly simple version of requests, keep the code understandable, reduce the footprint and not clutter the website.

[fejes713]

@mariastervic Thanks for suggestions and helping us solve this problem. @Chalarangelo I will update this PR in an approximately 2 hours, as soon as finish writing tests.

[fejes713]

@Chalarangelo time to close this finally. I've moved put and delete to the archive folder. I have updated httpGet and it works, I have also made changes to httpPost, you said you have some test environment for that so if you can please do test it. The tag_database is updated also.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.