search

pydio / pydio-core

Pydio 8 official repository
https://pydio.com
GNU Affero General Public License v3.0
867 stars 289 forks source link

Non-idiomatic use of const & let throughout the codebase #1405

Open jameswomack opened 6 years ago

jameswomack commented 6 years ago

Hey there! This is a cool project. I was looking at an old audio plugin on SourceForge and came across Pydio there. Anyway, in looking through your latest commits I see you've been updating old JavaScript to use modern syntax features like const (bravo) but not evenly. Using const & let instead of var is great, but inconsistently using them, particularly using let when const would work in a project that uses const, can make code worse off than just using var. That's because, unless you're writing in Assembly, code is more for humans than a computer, and let tells a human that the pointer is going to change. I've found the majority of component JS in this repo uses let for objects that have their fields changed even when the pointer doesn't change, which isn't necessary and is confusing as a reader. Example: https://github.com/pydio/pydio-core/blob/4d9e8c1cb196058e4e3e4908fa4909d7a07799d5/core/src/plugins/access.ajxp_conf/res/js/AdminWorkspaces/editor/WorkspaceCreator.js#L103

That line should use const, as should several similar LoC in the same component. Basically, anytime the pointer isn't reassigned to with =, you can use const.

cdujeu commented 6 years ago

ha ha, thanks james for pointing that out. We know about all that, but we don't have the current resources to go through ALL the code to fix that. That would definitely be a best practice, and that's why I'm trying to fix them along, when working on other fixes at the same time.

jameswomack commented 6 years ago

@cdujeu I totally understand, as someone who maintains a number of open source and private projects. The way I do this, is use the ESLint prefer-const rule and run eslint --fix to have the computer do it for me :)

cdujeu commented 6 years ago

Thanks for pointing to this. I would add that we are aware of many other similar "inconsistencies" in the js code :-). Like the React.createClass vs. extends React.Component constructors, as we have to keep the first form in many places to use our legacy mixins, etc...