JavaScript / jQuery code needs cleaning up

[koeaw]

Our code is full of incorrect JS comparisons and outdated variable declarations. This function declaration alone contains several instances of both.

Excerpt (example for both):

if (document.cookie && document.cookie != '') {
    var cookies = document.cookie.split(';');
...
}

Should be:

if (document.cookie && document.cookie !== '') {
    let cookies = document.cookie.split(';');
...
}

That first check if (document.cookie) seems nonsensical either way, looks like Python transferred to JS. Unless that's special jQuery magic I'm not aware of (which is entirely possible, as I've never written any jQuery myself).

[b1rger]

Can we make this a little bit more specific? Is there some linting that we can use so it is clear what "clean" and "not clean" means? If we can not check if an issue has been solved, it will just hang here forever...

[richardhadden]

What's wrong with this, apart from somewhat passé use of var instead of let?

(The if (document.cookie ...) is required, as no cookies being set returns undefined, not empty string. It's not JQuery magic!)

[koeaw]

@richardhadden Are you saying all the equality checks were purposefully written that way? I doubt that. When I see loose checks instead of strict ones in JS code which doesn't follow other conventions, that, to me, is an(other) indicator it was likely written by someone who didn't know better.

And I wasn't taking issue with verifying if the cookie exists, I was commenting on its implementation. if (document.cookie ...) is not how one would check if a variable was declared in JavaScript. In a Python-based project, it's a(nother) hint to me that the code was likely not written by someone who primarily works with JavaScript.

These things accumulate. The var/let thing could have just pointed towards the code being old, but in projects which aren't JS-focussed, it's not what I'd assume first.

[koeaw]

Relevant docs + choice quotes:

Equality comparisons and sameness:

Strict equality is almost always the correct comparison operation to use.

typeof operator and undefined:

One reason to use typeof is that it does not throw an error if the variable has not been declared.

[richardhadden]

I was about to mention the (absence of) strict equality checking :)

document.cookie isn't a variable, but part of the document object (so if it's not there, it's undefined, and doesn't throw an error). Maybe some browers don't define document.cookie until there is a cookie set? (I've tried out console on various pages, and got empty string and undefined).

Either way, we don't need to check whether it has a literal type of "undefined" — just whether it's there (otherwise we should also check it isn't null either). I'm tempted to argue we don't need the second part, as empty string is also falsy!

[koeaw]

document.cookie isn't a variable

Fair enough.

We both interpreted its intent to be to check for undefined cookies, in any case. And I'd argue there are better/more precise ways to check for undefined properties of objects as well. But ehh, looks like document.cookie isn't even a data property. :stuck_out_tongue_winking_eye:

And yeah, looks like it never does not exist in major browsers. I didn't even look at what the cookie does, so not sure if we even still need that check at all. The last cookie-related code I encountered was for the now-removed toggle between view/edit mode for entities, which is the kind of setting I'd use localStorage for, not cookies.

[b1rger]

Just for context: the code is apparently coming from this stackoverflow post: https://stackoverflow.com/a/23350233

It is used in the relations javascript to set the CSRF token for ajax form of the relations

[koeaw]

Just for context: the code is apparently coming from this stackoverflow post: https://stackoverflow.com/a/23350233

Interesting... especially considering content posted on SO is creative commons-licensed ("Subscriber Content" subsection).

[b1rger]

especially considering content posted on SO is creative commons-licensed

And the relevance of this in this context is..?

[koeaw]

And the relevance of this in this context is..?

Time spent discussing random copy-pasta feels like time wasted, to me. If the whole snippet was lifted from somewhere else, it's a bit pointless to discuss individual problems with it because the answer could simply be "laziness". Proper attribution would have made this clear right away.

Additionally, my assumption now is this probably wasn't a one-off thing. Which makes unknown n% of the remaining code equally kind of pointless to discuss with regard to style etc. And also doesn't make me feel too confident about other things.

[b1rger]

Can we make this a little bit more specific? Is there some linting that we can use so it is clear what "clean" and "not clean" means? If we can not check if an issue has been solved, it will just hang here forever...

q.e.d

[sennierer]

move all JS functions in dedicated *.js files in the static directory and run JS linters on these files:

• [ ] move all JS functions to JS files (in static directory)
• [ ] decide on a JS linter and run it on the code
• [ ] add the linter to the GH actions workflow