Remove jQuery from Webchat #2379
Conversation
leenagupte
force-pushed
the
remove-jquery-from-webchat-files
branch
from
948c504 to
bf963d6
Compare
|
Mentioning this for the record. The URL for HMPO is being updated in this PR: #2358 |
app/assets/javascripts/webchat.js
Outdated
| var $ = window.$ | ||
|
|
||
| $(document).ready(function () { | ||
| document.addEventListener('DOMContentLoaded', function () { |
There was a problem hiding this comment.
Recently had to do this exact change in another script and Kevin pointed out it would work just fine by removing the document.ready entirely without replacing it. Can you test whether this would work?
There was a problem hiding this comment.
You might also be able to to do the following:
(function () {
.... your code
})()There was a problem hiding this comment.
I think a self executing function in this context would be the same as having the code execute at this point without a document.ready.
There was a problem hiding this comment.
Ooh! Kevin's right. The event listener isn't needed. I'll remove it and fix up.
| @@ -27,14 +23,19 @@ | |||
| if (!availabilityUrl || !openUrl) { | |||
| throw Error.new('urls for webchat not defined') | |||
| } | |||
| $openButton.on('click', handleOpenChat) | |||
|
|
|||
| if (redirectUrl === true) { | |||
There was a problem hiding this comment.
Just curious why this if statement wraps the button add listener? I don't see the equivalent in the original code.
There was a problem hiding this comment.
Ah, I think I was being overly cautious, because the "open button" only exists when data-redirect is true. However, you are right it in that it's not needed. I'll swap it for your recommendation below. Thanks!
| @@ -27,14 +23,19 @@ | |||
| if (!availabilityUrl || !openUrl) { | |||
| throw Error.new('urls for webchat not defined') | |||
There was a problem hiding this comment.
Not your code, but I wonder if we should take this opportunity to improve this error handling. Kevin suggested something like this on a bit of work I did recently:
throw Error.new('urls for webchat not defined', window.location)
There was a problem hiding this comment.
Would passing window.location.href be more useful than using window.location? The window.location object seems to have a lot of stuff in it:
There was a problem hiding this comment.
I'll add this change to a new commit.
| var currentState = $el.find('.' + webchatStateClass + state) | ||
| $el.find('[class^="' + webchatStateClass + '"]').addClass('govuk-!-display-none') | ||
| currentState.removeClass('govuk-!-display-none') | ||
| var currentState = el.querySelectorAll('[class^="' + webchatStateClass + state + '"]')[0] |
There was a problem hiding this comment.
In jQuery, .find() finds everything that matches. In JavaScript, it's weirdly more complex
querySelectorfinds the first element that matchesquerySelectorAllfinds all elements that match
If you do querySelectorAll()[0] that returns the first one found - it's equivalent to doing querySelector. Which one do you want to do here? I suspect it's querySelector?
There was a problem hiding this comment.
🤦♀️ Yes, I just need querySelector.
leenagupte
force-pushed
the
remove-jquery-from-webchat-files
branch
from
bf963d6 to
0208bbe
Compare
|
@andysellick Thanks for your review. I've made the recommended changes and fixed up. |
|
When #2358 is merged tomorrow, Webchat will start working again. At that point I will rebase and make sure that this change hasn't broken anything. |
leenagupte
changed the title
This will be required to fake responses from XMLHttpRequest once jQuery has been removed from webchat.js and webchat/library.js as we will no longer be able to spyOn ajax and create a fake response.
The linter threw an error as the creation of a new GOVUK.Webchat object is not being stored in a variable. Storing a variable isn't appropriate in this case as it would not be used, so the linter is being ignored. See: https://stackoverflow.com/questions/33287045/eslint-suppress-do-not-use-new-for-side-effects
The "ready" function in jQuery waits for the DOM tree to load, i.e. be "ready" before moving on. The equivalent to this in vanilla JS is to listen for the "DOMContentLoaded" event. However, this script works without an event listener on "DOMContentLoaded", so the ready function has just been removed.
leenagupte
force-pushed
the
remove-jquery-from-webchat-files
branch
from
0208bbe to
787c8bf
Compare
|
It's currently showing "Advisors are busy" which matches live. I would like to see the pop-up open when advisors are available before merging.
|
|
Oh dear. It's seems that the "Advisors are available" message shows. But the link doesn't link to anywhere 😱 |
|
Success! I was defining var openButton = document.getElementsByClassName('.js-webchat-open-button')[0]it should have been: var openButton = document.querySelector('.js-webchat-open-button')
I'll update and fix up. |
leenagupte
force-pushed
the
remove-jquery-from-webchat-files
branch
from
787c8bf to
c4e5ba6
Compare
leenagupte
changed the title
This commit replaces like for like jQuery methods with vanilla JS. Ajax will be removed in a separate commit.
Creates an XMLHttpRequest object to make the request to the availability
url. We need to use XMLHttpRequest rather than the new "await" and
"fetch" functions as they are not supported in Internet Explorer.
There is only one webchat page that this script runs on:
/government/organisations/hm-passport-office/contact/hm-passport-office-webchat
The [availabilityURL] always returns a response of:
```
{
"status": "success",
"response": "UNAVAILABLE"
}
```
with an http status of 200. This seems incorrect as the [openUrl] states
that all advisors are busy, so we would expect the availabilityURL to
return a status of "BUSY". There is an [open ticket] to change the URLs,
so hopefully that will fix the problem. The removal of jQuery in this
change has not negatively affected the workings of Webchat. The
behaviour hasn't been affected, even if that behaviour is incorrect.
The XMLHttpRequest is still being called asynchronously like the old
ajax call ("true" in the call to request.open).
`readyState` and the `status` returned by the XMLHttpRequest object
are used to determine whether the request has been successful. That
means how the response is being stubbed in the tests has been changed to
match.
The "mount" function is doing the equivalent of the webchat.js script,
so it has been updated to match that script.
The jsonNormalisedError object had to be updated in the tests, as a
string without a status was not an accepted response for
XMLHttpRequest.
As ajax has been removed, it can no longer be spied on in the tests.
However, the tests still use jQuery, so jasmine Ajax (installed in a
prior commit) can be used define fake responses from XMLHttpRequest.
XMLHttpRequest makes two calls, to open and send, it was easier to stub
the response of the most recent call rather than trying to mock both.
This means that in test "should only track once per state change" that
tests the URL polling, we only need to change the "most recent" ajax
stub when the response being returned changes. It was much simpler to
just call the "most recent" stub twice than try to replicate the
operation of the fake function.
[availabilityURL]: https://hmpowebchat.klick2contact.com/v03/providers/HMPO/api/availability.php
[openUrl]: https://hmpowebchat.klick2contact.com/v03/launcherV3.php?p=HMPO&d=717&ch=CH&psk=chat_a1&iid=STC&srbp=0&fcl=0&r=Chat&s=https://hmpowebchat.klick2contact.com/v03&u=&wo=&uh=&pid=2&iif=0
[open ticket]: #2358
This will allow us to pinpoint which GOV.UK pages are trying to open a webchat url, without having any urls defined in [webchat.yaml] [webchat.yaml]: https://github.com/alphagov/government-frontend/blob/5974c4ec2c7e7a4eedcfda8f48370cce79c8efa0/lib/webchat.yaml
leenagupte
force-pushed
the
remove-jquery-from-webchat-files
branch
from
c4e5ba6 to
5a5e77d
Compare
Trello: https://trello.com/c/jymNmjHN
What's changed?
Remove jQuery from webchat/library.js and webchat.js.
This change is very large, so the initialisation of the code has not been changed, neither has Webchat been converted into a GOVUK module. The focus of this change is just to remove the reliance on jQuery in the Webchat scripts which is a large change by itself.
An anomaly was discovered during the testing of this change.
There is only one webchat page that this script runs on:
/government/organisations/hm-passport-office/contact/hm-passport-office-webchat
The availabilityURL always returns a response of:
with an http status of 200. This seems incorrect as the openUrl states that all advisors are busy, so we would expect the availabilityURL to return a status of "BUSY".
There is an open ticket to change the URLs, so hopefully that will fix the problem. The removal of jQuery in this change has not negatively affected the workings of Webchat. The behaviour hasn't been affected, even if that behaviour is incorrect.
The response stubbed from the availabilityURL has also been changed in the tests as the switch from using Ajax to calling XMLHttpRequest requires more information from request object (http status code and readyState) to determine when the request has been completed.
Results from testing