onebeyond / systemic

📦 A minimal dependency injection framework.
https://onebeyond.github.io/systemic
MIT License
56 stars 7 forks source link

Upgrade syntax - why and what? #42

Closed w3dani closed 3 years ago

w3dani commented 3 years ago

The goal of this issue is to discuss the need of upgrade the systemic code syntax and what we will change. (see https://github.com/guidesmiths/systemic/pull/41)

From my point of view, there are three things to change:

I think there are no discussion on use let and const because every IDE discourage the usage of var, and we don't need to talk about the callbacks readability, but the discussion about using function or not could be funnier.

Just to give some extra-context, I think most of the projects on Guidesmiths that are using arrow functions so we are more used to use that syntax. btw, some docs about the differences https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

Would you apply all these changes to this repo? Why?

btw: this is my first issue on github, if I'm missing something please let me know 😄

cressie176 commented 3 years ago

Replace function keyword with arrow functions

I agree with this when they're inline and we can be sure that there won't be any issues with this, e.g. here and here are fine, but not when it would require the use of the const keyword, as it would here

const start = (cb) => {
 // ...
}

ends up being less readable due to all the hieroglyphics.

Replace var keyword with let/const

Agree, although not because some IDEs discourage it - that's the tail wagging the dog. The reason to prefer let is because the scoping rules adhere more closely to the principle of least astonishment

Replace callbacks with promises/async-await

Systemic works with both promises/async-await and callbacks (and should continue to do so). Which callbacks do you propose to replace?

I would also add change from eslint/imperative to esnext with the following rules

{
  "env": {
    "node": true,
    "es2015": true,
  },
  "extends": "esnext",
  "parserOptions": {
    "ecmaVersion": "es2015"
  },
  "rules": {
    "indent": ["error", 2],
    "import/no-commonjs": "off",
    "import/no-nodejs-modules": "off",
  }
}

Then most of the changes can be applied automatically using eslint . --fix

cressie176 commented 3 years ago

I've switch eslint to use esnext and --fixed the syntax