enable all eslint rules and fix problems #297
Conversation
See discussion in tus#291 - had to disable no-use-before-define many places where onSuccess uses the upload object, maybe in the future onsuccess could provide upload as an argument to onSuccess - also a lot of functions moved (as eslint doesn't allow function hoisting) - disabled eslint for demos/cordova and demos/browser
|
any chance of getting this merged @Acconut before we get too many conflicts? Or are there any rules you don't agree with that you would like to disable? |
| @@ -1,5 +1,6 @@ | |||
| /* eslint no-console: 0 */ | |||
|
|
|||
| // eslint-disable-next-line import/no-extraneous-dependencies | |||
There was a problem hiding this comment.
Do you know why import/no-extraneous-dependencies has to be disabled here? browserstack-runner is defined as a dependency in package.json
There was a problem hiding this comment.
Oh, it's because it's only a devDependency. I think that rule's purpose is to help catch errors where we accidentally try to use a module that we forgot to add to dependencies, only to realise it after publish, or a user realises that it's missing. In this case browserstack-jasmine.js is indeed just a local dev script so it's not part of the produced code, so it's ok to use devDependencies. I think this could be solved either by disabling each use of dev deps like we do here, or by setting up different eslint config for different paths, where we disalbe this rule for dev code.
| @@ -6,6 +6,7 @@ | |||
| * according to the result. | |||
| */ | |||
|
|
|||
| // eslint-disable-next-line import/no-extraneous-dependencies | |||
There was a problem hiding this comment.
Again, is it necessary to disable import/no-extraneous-dependencies here?
| } else { | ||
| try { | ||
| data = !data.trim().length ? {} : JSON.parse(data) | ||
| const data2 = !data.trim().length ? {} : JSON.parse(data) |
There was a problem hiding this comment.
I assume there is a rule disallowing the reassignment of arguments. Could we loosen up this rule? I actually think the previous code where reassignment is allowed is more reasonable. Or is there a specific reason against it?
There was a problem hiding this comment.
You can see the rationale behind the rule here: https://eslint.org/docs/rules/no-param-reassign
Although I agree that it's not a big problem, so we can disable that rule if you like.
lib/upload.js
Outdated
| // Count the number of arguments to see if a callback is being provided as the last | ||
| // argument in the old style required by tus-js-client 1.x, then throw an error if it is. | ||
| // `arguments` is a JavaScript built-in variable that contains all of the function's arguments. | ||
| if (arguments.length > 1 && typeof arguments[arguments.length - 1] === 'function') { | ||
| if (arguments.length > 1 && typeof args[arguments.length - 1] === 'function') { |
There was a problem hiding this comment.
Could you explain this change? Are we not allowed to access arguments, but we can use arguments.length?
There was a problem hiding this comment.
TBH I can't explain that 😅 It seems like the rule doesn't enforce all uses of arguments, I don't know why. I'll remove the other uses too. Or if you think this rule should be disabled we can do that too.
even though eslint didn't catch them for some reason
See discussion in #291