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

$.Ajax not available in staticman.js #766

Closed
sidharthramesh opened this issue Jan 19, 2021 · 15 comments · Fixed by #782
Closed

$.Ajax not available in staticman.js #766

sidharthramesh opened this issue Jan 19, 2021 · 15 comments · Fixed by #782

Comments

@sidharthramesh
Copy link

When trying to submit comments through staticman, the form posts to the same page and results in a 405 not allowed (since github doesn't allow posts)

The error on the console is:

staticman.js:14 Uncaught TypeError: $.ajax is not a function
    at HTMLFormElement.<anonymous> (staticman.js:14)
    at HTMLFormElement.dispatch (VM494 jquery-3.5.1.slim.min.js:2)
    at HTMLFormElement.v.handle (VM494 jquery-3.5.1.slim.min.js:2)

Maybe the slim version of jQuery doesn't include ajax?

<!-- doing something a bit funky here because I want to be careful not to include JQuery twice! -->
<script>
if (typeof jQuery == 'undefined') {
document.write('<script src="https://code.jquery.com/jquery-3.5.1.slim.min.js"></scr' + 'ipt>');
}
</script>
<script src="{{ "/assets/js/staticman.js" | relative_url }}"></script>

As a result, none of these gets called, and the defaultAction of the form is to post on the same page. Which results in a 405 error.

{% assign sm = site.staticman -%}
var endpoint = '{{ sm.endpoint | default: "https://staticman3.herokuapp.com/v3/entry/github/" }}';
var repository = '{{ sm.repository }}';
var branch = '{{ sm.branch }}';
$.ajax({
type: $(this).attr('method'),
url: endpoint + repository + '/' + branch + '/comments',
data: $(this).serialize(),
contentType: 'application/x-www-form-urlencoded',
success: function (data) {
$('#comment-form-submit').addClass('d-none');
$('#comment-form-submitted').removeClass('d-none');
$('.page__comments-form .js-notice').removeClass('alert-danger');
$('.page__comments-form .js-notice').addClass('alert-success');
showAlert('success');
},
error: function (err) {
console.log(err);
$('#comment-form-submitted').addClass('d-none');
$('#comment-form-submit').removeClass('d-none');
$('.page__comments-form .js-notice').removeClass('alert-success');
$('.page__comments-form .js-notice').addClass('alert-danger');
showAlert('failure');
$(form).removeClass('disabled');
}
});
return false;

@daattali
Copy link
Owner

daattali commented Jan 19, 2021 via email

@sidharthramesh
Copy link
Author

I saw your comment regarding including jQuery twice. How does this happen? I don't want to include it twice and make the page slower.

@daattali
Copy link
Owner

daattali commented Jan 19, 2021 via email

@sidharthramesh
Copy link
Author

I like the fact that the pages load fast. However, I'm working on optimising it even more.

Is $.ajax really required here? How about using fetch or axios here instead of the full version of jQuery? Including a very common version of jQuery from a CDN will be another alternative since it is almost a given that the browser will have a copy of it in its cache.

@daattali
Copy link
Owner

I'm not an expert in staticman. You can look at the staticman javascript file to see where/why it's needed

https://code.jquery.com/jquery-3.5.1.slim.min.js is the version that's being loaded and it's the officially recommended CDN by jQuery, are you saying that's not a good CDN?

@daattali
Copy link
Owner

daattali commented Feb 4, 2021

Perhaps @VincentTam has something useful to say here

@VincentTam
Copy link
Contributor

VincentTam commented Feb 4, 2021

When I ported @mmistakes's code, I was using the min version. Tracing back the changes, I see that it has been replaced to a slim version at commit 5991966#diff-e9c677c6d71b6acd8eb0963e0e2a9bd997a86240a3711c2d391c42d18eb12c32.

I'm learning JavaScript, and I'll know $.ajax in a month or so.

Is $.ajax really required here? How about using fetch or axios here instead of the full version of jQuery?

@sidharthramesh AFAIK, it's possible to use Staticman without jQuery. However, @onweru nice work that takes time to digest. Lemme come back to this when I finished rewriting his theme's Wiki page about Staticman.

@daattali
Copy link
Owner

daattali commented Feb 4, 2021

You're right looks like I'm at fault for slim-mifying jquery. When I refactored I noticed that slim version is fine for all the parts of the site that I built, I didn't think to consider staticman. Let us know when you find out

@VincentTam
Copy link
Contributor

VincentTam commented Feb 4, 2021

I respect your decision to use slim, and I like your "minimal" concept in the README. @staticmanlab and I would be sad if the jQuery gets fat because of us.

I'll soon search for the functional difference between the two versions of jQuery. As @onweru has worked out a pure-JS implementation of Staticman integration in his Hugo theme for which I has been invited to a collaborator (c.f. the 2nd external link in my previous comment), it would be nice if I can decipher his code and find a slimmer version. Lemme try this this/next week-end.

@VincentTam
Copy link
Contributor

VincentTam commented Feb 12, 2021

@sidharthramesh @daattali I fixed the code a few days ago by replacing $.ajax method with fetch() in standard JS. Please verify and merge, so that the reported problem would be fixed.

P.S. Staticman officially updated its website to include setup guide for v3 yesterday. https://staticman.net

@sidharthramesh
Copy link
Author

Hey @VincentTam thank you! I considered using fetch as well, however not all browsers support it. Especially IE. So perhaps a solution using XHR will be better for comparability? Axios can be another alternative. What do you think @daattali ?

@VincentTam
Copy link
Contributor

@sidharthramesh Thanks for suggestion. I prefer your way. I've worked it out. Please verify.

@sidharthramesh
Copy link
Author

Great work @VincentTam. I'll be testing it and using this PR on my fork. @daattali please consider merging and closing this issue.

@VincentTam
Copy link
Contributor

Hey @VincentTam thank you! I considered using fetch as well, however not all browsers support it. Especially IE.

I'm posting links to MDN docs about the browser compatibilities for quick reference.

  1. https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#browser_compatibility no IE support 😭
  2. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#browser_compatibility
  • XMLHttpRequest() constructor: 7
  • onreadystatechange: 5
  • readyState: 7
  • readyState.constants: 9
  • send: 5
  • setRequestHeader: 5

I've used XMLHttpRequest.DONE in my 2nd proposed PR, so the min version needed is IE9. I prefer readability over a marginal gain of backward compatibility. Who's using IE8 nowadays?

var xhr = new XMLHttpRequest();
xhr.open("POST", url);
xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded');
xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
xhr.onreadystatechange = function () {
if(xhr.readyState === XMLHttpRequest.DONE) {
var status = xhr.status;
if (status >= 200 && status < 400) {
formSubmitted();
} else {
formError();
}
}
};

@daattali
Copy link
Owner

I think IE11+/edge is reasonable, anyone using IE10 or below is probably having problems with half the internet, so I wouldn't worry about that. Too bad that IE never implemented fetch support. I'm sorry for the long delay, will look at the PR now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants