WIP - eslint & code refactoring

[PopGoesTheWza]

@koresar all done except packages/check-compose

• only the package-lock.json required by CI Travis are generated and commited to the repo
• every package has been refactored and has use strict at the top (there is a single exception which requires more attention, packages/convert-class/index.js)
• __test__ scripts have been only "fixed" using eslint with no other code change
• ES2016 syntax everywhere and deprecated items like __proto__ are gone
• Interaction with objects are primarily done using Reflect and Object methods
• packages/collision/index.js would still benefit from some more love for Collision is composed using some composer-from-hell which I've not fully reduced

[PopGoesTheWza]

addresses #57 and #58

[koresar]

Here is a general feedback from my 7 years of JavaScript experience

(and 8 years more of other stuff)

TL;DR: increase in maintenance complexity for no visible (to me) benefits.

I've seen OSS project stagnate because none could maintain it. This happens daily. To avoid that pitfall I developed a set of practices to make sure my projects survive. In general - KISS (keep it stupid simple). Thus this project had so few configuration files and additional tools/modules installed. Every new (dev) dependency is the maintenance overhead, maybe not now but in months time.

Secondly. The ESLint rules used are for production browser bundled apps (namely - airbnb). Whereas stampit source code is a low level JS library.

As a hint of wrongly chosen ESLint rules - see how many times you had to add eslint-disable. I counted 78. Wow! So much work. But what's the benefit? To satisfy airbnb? The ESLint rules better be chosen to satisfy our project needs.

Take this low level lib for example - Preact. Here is their entire ESLint config - https://github.com/preactjs/preact/blob/master/package.json#L42 This config more suitable for stampit as well.

=====

If you still believe that airbnb was the right choice and you would like to continue maintaining it - then I'm okay with the ESLint configuration.

=====

On the non-ESLint front - well done! That's a progress to a right direction. Thank you for the effort.

[koresar]

Loving your story mate! 👍 Feel free to experiment here, I see why you need this now. Apologies if I was somewhat grumpy.

On Mon., 11 Nov. 2019, 21:36 PopGoesTheWza, notifications@github.com wrote:

@PopGoesTheWza commented on this pull request.

In package.json https://github.com/stampit-org/stamp/pull/73#discussion_r344649618:

}, "engines": { "node": ">= 8.2.1" }, "devDependencies": {

• "eslint": "^6.6.0",
• "eslint-config-airbnb-base": "^14.0.0",

Now another story:

I never bothered about eslint for most if not all the projects I own or contribute to are Typescript based, and thus relying on tslint.

I only recently found out that the Typescript team has officially deprecated tslint and that the whole TS ecosystem should now go the eslint way.

So adding eslint to @stamp is both

• an opportunity for me to get familiar with it
• a necessary intermediate step if we choose to convert to Typescript

The current eslint setup and set of active rules should not remain so strict and extended.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/pull/73?email_source=notifications&email_token=AAMMEL5UXZAXPRL4ZSIX66TQTEYRXA5CNFSM4JKCEZ5KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLB3JJI#discussion_r344649618, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL2BQTGFJAQBY5DP7D3QTEYRXANCNFSM4JKCEZ5A .

[koresar]

AFAIK, the places where we use "this" the script mode is still necessary. But surely not in every way file. It's sorta was deprecated in ES6 i think.

On Mon., 11 Nov. 2019, 21:51 PopGoesTheWza, notifications@github.com wrote:

@PopGoesTheWza commented on this pull request.

In packages/it/index.js https://github.com/stampit-org/stamp/pull/73#discussion_r344655679:

@@ -1,97 +1,99 @@ -var compose = require('@stamp/compose'); -var Shortcut = require('@stamp/shortcut'); -var isStamp = require('@stamp/is/stamp'); -var isString = require('@stamp/is/string'); -var isObject = require('@stamp/is/object'); -var isFunction = require('@stamp/is/function'); -var merge = require('@stamp/core/merge'); -var assign = require('@stamp/core/assign');

-var concat = Array.prototype.concat; -function extractFunctions() {

• var fns = concat.apply([], arguments).filter(isFunction); +/ eslint-disable no-use-before-define /

• +'use strict';

Now that you mention it, use strict is not necessary for node scripts and commonjs modules. I'd need to check with Typescript still.

I have to habit of always writing and running in strict mode

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/pull/73?email_source=notifications&email_token=AAMMEL6PRZD2VMN6QWAIPWLQTE2LTA5CNFSM4JKCEZ5KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLB5FOA#discussion_r344655679, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL46GUI3C5VTWOSRRCDQTE2LTANCNFSM4JKCEZ5A .

[PopGoesTheWza]

As a hint of wrongly chosen ESLint rules - see how many times you had to add eslint-disable. I counted 78. Wow! So much work. But what's the benefit? To satisfy airbnb?

Following my update from yesterday, I count a total of 105 eslint-disable occurrences.

• 99 of them are in __test__ and check-compose files for which I only applied minimal editing (i.e. to pass prettier and lint.) The vast majority of these relates with jest best practices for what I gathered. Since unit testing is not my strong point and my knowledge of jestis minimal, I have not investigate further. Still, my feeling is that unit tests usually benefit from any mecanics (like linting rules) which enforce good quality unit test. We'll all seen too many projects with poorly designed unit tests which only enable the project manager to check a mark on his dashboard "unit tests: Done")
• 1 eslint-disable-next-line no-nested-ternary (in packages/collision/index.js): reasoning being that nested ternary should be used sparingly and require the developer to manually override the rule when it make sense.
• 1 eslint-disable-next-line no-unused-vars (in packages/convert-class/index.js): reasoning being that unused vars should occur for a reason and require the developer to manually override the rule when it make sense.
• 2 eslint-disable global-require (in packages/core/index.js and packages/is/index.js: reasoning being that require() should be global and preferably at the top. This rule is likely to become irrelevant in Typescript.
• 2 eslint-disable no-use-before-define (in packages/it/index.js and packages/required/index.js): I have not yet fully read this rule and thus decided on its relevancy.

If you still believe that airbnb was the right choice and you would like to continue maintaining it - then I'm okay with the ESLint configuration.

I find airbnb base package to be an easy and quick way to get started, but after reading your feedback, I think it might be relevant at one point to explicitly move airbnb rules directly within .eslintrc.json (to prevent any "blackbox" effect when the airbnb package updates) and then drop the airbnb package entirely.

[PopGoesTheWza]

Apologies if I was somewhat grumpy.

No worries.

[koresar]

I've disabled all the jest rules for check-compose folder. We should be good now. :)

The number of eslint-disable dropped 105->31. Still too much as for that number of files, but we can merge now. :)

[koresar]

I still don't like the fact that I had to add configuration to disable other configuration. This is configuration inception. :)

[PopGoesTheWza]

That’s meta linking.