JavaScript is messy & confusing (JS guidelines needed) #124
4 participants
Yes, copypaste, I am aware that the main.js file is big. There's a reason I've build them all as self-contained widgets. I am inevitably going to split them out as different files.
The issue is that I still have yet to have time to fully focus on refactoring the widget system. I've managed to expand some of its featureset, but the actual widget loader itself is behind the times. What's going to happen is you will have ...
app/widgets.js and app/widget/xxx.js. Each time a widget is loaded, it's going to check its weight property (for order of operations) and a method like canLoad that will allow arbitrary code to run before attempting to load it, checking if other widgets have loaded or if the browser can support its feature-set for graceful degradation.
I'll probably borrow a lot from that readme.
It's more than a README, it also includes a build system...https://github.com/Cipherwraith/2ch/blob/master/gruntfile.js
(Of course less complex than the one this project would require since the only two locale options are English and Japanese)
Your idea seems to be to move dependency management from build time to run time. We'll see how that works out :)
How do you intend to police the codebase for XSS vulnerabilities?
Well you close holes as they arise in an application, preferably during a beta period...genuinely curious, what other method is there??
Testing is nice but not a complete solution. You want to be able to easily audit the sources and sinks of untrusted data. JQuery in particular introduces a lot of sinks that need to be watched. It would probably be a good idea to use a template system instead of lots of
$('<span>' + string + '</span>').
Are you planning on doing the user JS thing again? Stuff like that makes a Javascript injection much more serious because the attacker can put phone-home code into the users' settings. Userscript engines like Greasemonkey are more secure because user interaction is required to install a script.
SQL injection is a whole other thing, but good to hear you're covered on that front.
Are you planning on doing the user JS thing again?
No. Use Tamper/GreaseMonkey. That was a mistake.
Also, most of what I wrote in the OP has been fixed by @jaw-sh.
Good to hear about the user JS.
Would you be okay with a PR changing innerHTML concatenation to DOM methods?
I'm changing basically all the JS right now at this moment. Point out line numbers with what you're talking about please.
Just a quick scan, probably not catching everything:
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L127
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L207
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L208
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L303
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L311
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L318
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/postbox.widget.js#L441
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/donate.widget.js#L113
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/widgets/donate.widget.js#L348
https://github.com/infinity-next/infinity-next/blob/master/public/static/js/app/main.js#L315
There's also some instances of this in the jQuery plugins.
Right now we have a 1500 line
main.js.https://github.com/infinity-next/infinity-next/blob/master/public/js/app/main.js
One of the things 8chan did right was separating different features into different scripts, then the build program concatenates them together and minifies them all, see our tree:
Site owners can easily enable/disable features just by adding or removing new scripts to the array.
https://github.com/ctrlcctrlv/infinity/tree/master/js (index of all scripts)
https://github.com/ctrlcctrlv/infinity/blob/master/inc/instance-config.php#L87 (script listing on the public site)
8chan has over 80 scripts written by more than 12 different people over the course of 5 years, yet we can understand the whole thing, and add more and edit them even today. This is because each feature we would want to change is in its own script, all it takes is finding it in the directory listing.
Now I'm not saying that this is perfect, but recently I was tasked with a rewrite of 2ch JS. These are the guidelines I came up with: https://github.com/Cipherwraith/2ch/
I think this is much closer to perfection, the numeric key loading especially helps with dependency management, a sore spot in the infinity tree.
As Laravel's Elixir already requires NPM, I think we should either switch to
grunt(or implement my gruntfile above in Elixir).