v1 improvements/clarifications

[mifi]

I went through the rule overrides inside index.js and here are my comments:

1. 'react/jsx-handler-names': ['off'] - Isn't this one already off in airbnb-base, so no need to turn it off here? https://github.com/airbnb/javascript/blob/9c181108a7f11be1b97191f17c9ab2dc98e28c6e/packages/eslint-config-airbnb/rules/react.js#L83
2. 'react/react-in-jsx-scope': 'error', - Isn't this one already error in airbnb-base, so no need to off it here? https://github.com/airbnb/javascript/blob/9c181108a7f11be1b97191f17c9ab2dc98e28c6e/packages/eslint-config-airbnb/rules/react.js#L229
3. 'array-callback-return': 'warn' - I think this one would be nice to enforce to reduce confusion when reading code and prevent developer from forgetting to return a value
4. no-unused-vars - I think this should be on. I don't know the use case of having unused variables declared (if testing code, we can comment out instead, or eslint disable line)
5. 'max-len': 'warn' - I don't like warnings in general, they tend to build up and then you just ignore them and never fix any of them. IMO either use error, or disable the rule altogether
6. key-spacing I understand that you really like the alignment of property keys, but I find this rule a bit anti-productive. Everytime when changing the longest key name, I need to also fix spacing on the other keys. Maybe it can be solved automatically with prettier or similar, we could look into that. Another issue is that it leads to unneccesarily long commit diffs.
7. quotes.allowTemplateLiterals: true - I'm not sure why this is desired? With the default value false it will still allow backticks when there is a variable inside or for multiline strings
8. comma-dangle why is this disabled for imports, exports and functions only? The default (always-multiline) reduces commit diffs in many cases when adding/removing lines of code
9. dot-notation IMO this is subjective, I don't think that state['asdf'] reads better than state.asdf
10. camelcase I'm guessing we are using different cAs_InG in a lot of code and it's too much work (and too risky) to change this? or you prefer mixing casing in variables?
11. no-console while it's ok for node.js apps to console.log, it's not generally true for browser/client apps like React, so I'm not sure if it should default to off for all projects
12. no-fallthrough I'm guessing too much work to enable this
13. node/no-path-concat - I think this one is a good rule as it can make code work on non-unix systems too (maybe not a priority for transloadit but it's a good practice). And it can prevent accidental paths joining with double or missing slashes. e.g. '/foo/bar/' + '/file.jpg' leads to /foo/bar//file.jpg and '/foo/bar' + 'file.jpg' leads to /foo/barfile.jpg
14. why is 'react/no-unused-prop-types': 'error' while 'react/prop-types': ['off'] ? can we remove the first one?
15. 'react/require-render-return': ['off'] What's the reason for disabling this? Correct me if I'm wrong but I think react will crash if render() returns undefined so it's good to guard against?
16. 'no-template-curly-in-string': ['off'] I'm sure there's a good reason for disabling it, but if not then this rule could prevent accidentally using ${variable} in a normal string instead of backticked string
17. 'no-useless-escape': ['off'] - I think u\nn\e\cc\e\s\\ar\ily escapes makes it harder to read regexes and other code. why is this turned off?
18. 'no-continue': ['off'] - I think continue can usually be rewritten to a function (return) or if statements but if you prefer using continue i'm not much against that either.
19. 'arrow-body-style': ['off'] not a big issue but I'm not sure when it's useful to have a single long line with the return statement inside (if that was the usecase)
20. 'no-cond-assign': ['off'] I agree that experienced developers should know the difference between = and ==/===, but in the years before eslint I did actually mistype this and pulling my hair out wondering why it doesn't work. Maybe we instead could useeslint-disable-next-line explicitly when we know that we want to assign inside a conditional?
21. 'react/jsx-closing-tag-location': ['off'] I think that XML reads better and is easier to navigate when end tag is aligned with starting tag. If eslint --fix causes breakage, maybe better to disable the rule for the offending project or file and not disable globally in our central config eslint
22. 'import/extensions': ['warn'] maybe move this warning rule into projects that have a lot of require('xx.js')?
23. 'no-alert': ['warn'] - alert is really not good to use in user-facing code, maybe should instead move this warn into files or projects that need to use "alert" (i'm guessing internal applications), and not have it a a global warn
24. 'no-restricted-properties': ['warn'] I understand that many projects are using deprecated globals, but maybe this rule should be disabled on those projects instead of being disabled for all other (and new) projects as well
25. 'default-case': ['warn'] not so fan of warnings as it will drown in a sea of other warnings. IMO either disable in offending projects or remove this rule completely

Also I see that typescript was added - why is it a necessary dependency?

[kvz]

1. 'react/jsx-handler-names': ['off'] - Isn't this one already off in airbnb-base, so no need to turn it off here? https://github.com/airbnb/javascript/blob/9c181108a7f11be1b97191f17c9ab2dc98e28c6e/packages/eslint-config-airbnb/rules/react.js#L83
2. 'react/react-in-jsx-scope': 'error', - Isn't this one already error in airbnb-base, so no need to off it here? https://github.com/airbnb/javascript/blob/9c181108a7f11be1b97191f17c9ab2dc98e28c6e/packages/eslint-config-airbnb/rules/react.js#L229

👍

3. 'array-callback-return': 'warn' - I think this one would be nice to enforce to reduce confusion when reading code and prevent developer from forgetting to return a value
4. no-unused-vars - I think this should be on. I don't know the use case of having unused variables declared (if testing code, we can comment out instead, or eslint disable line)

Moved the warnings to the api repo, so we can fix and remove override locally

5. 'max-len': 'warn' - I don't like warnings in general, they tend to build up and then you just ignore them and never fix any of them. IMO either use error, or disable the rule altogether

we could do that in a next major, but i think warnings are helpful as a grace period, we can't fix everything in one moment.

6. key-spacing I understand that you really like the alignment of property keys, but I find this rule a bit anti-productive. Everytime when changing the longest key name, I need to also fix spacing on the other keys. Maybe it can be solved automatically with prettier or similar, we could look into that. Another issue is that it leads to unneccesarily long commit diffs.

VS Code does this automatically upon save for me, i think fixing people's setups in this way prevents most of the friction while improving readability.

7. quotes.allowTemplateLiterals: true - I'm not sure why this is desired? With the default value false it will still allow backticks when there is a variable inside or for multiline strings

We have a lot of code defaulting to backticks, and that makes it easy to add errors or variables later on. Also it can look more consistent when you have 5 lines assigning values and only 3 of them use backticks but 2 use single quotes.

8. comma-dangle why is this disabled for imports, exports and functions only? The default (always-multiline) reduces commit diffs in many cases when adding/removing lines of code

👍 changed

9. dot-notation IMO this is subjective, I don't think that state['asdf'] reads better than state.asdf

I think so too, which is also the point that Evgenia made, that I imported from the content repo. I think we are in agreement and it can remain off? Or maybe i misunderstand.

10. camelcase I'm guessing we are using different cAs_InG in a lot of code and it's too much work (and too risky) to change this? or you prefer mixing casing in variables?

I think there are a lot of cases in the api, where we take values like {assembly_id} from api communication with the outside world, can can then easily follow along that var, instead of assigning it to something else because: camel is better. I also think all transloadit coders have already established to use camel in other cases though. I haven't seen awkward casing in the wild when there wasn't a similar reason for it, i think.

11. no-console while it's ok for node.js apps to console.log, it's not generally true for browser/client apps like React, so I'm not sure if it should default to off for all projects

Agree 👌

12. no-fallthrough I'm guessing too much work to enable this

But also a question about its usefulness, i have seen legitimate cases for using fallthroughs and haven't seen a critical bug due to it yet i think 🤔

13. node/no-path-concat - I think this one is a good rule as it can make code work on non-unix systems too (maybe not a priority for transloadit but it's a good practice). And it can prevent accidental paths joining with double or missing slashes. e.g. '/foo/bar/' + '/file.jpg' leads to /foo/bar//file.jpg and '/foo/bar' + 'file.jpg' leads to /foo/barfile.jpg

For the time being, our focus is on linux/macos/bsd:

I think a longterm goal can be fixing it, but it is adding a whole bunch of characters to our scripts until then while they won't be windows compatible even after adding them. I think maybe for a major v5, we can turn it back on for instance

14. why is 'react/no-unused-prop-types': 'error' while 'react/prop-types': ['off'] ? can we remove the first one?

Good catch, i mostly copied over Evgenia's setup from content regarding the react things, i have little experience there.

15. 'react/require-render-return': ['off'] What's the reason for disabling this? Correct me if I'm wrong but I think react will crash if render() returns undefined so it's good to guard against?

Unsure, same reason as above, removed it.

16. 'no-template-curly-in-string': ['off'] I'm sure there's a good reason for disabling it, but if not then this rule could prevent accidentally using ${variable} in a normal string instead of backticked string

iirc, it would prevent us from doing '{YOUR_AUTH_KEY}', but we have a lot of those, and escaping all of them is less favorable imho

17. 'no-useless-escape': ['off'] - I think u\nn\e\cc\e\s\ar\ily escapes makes it harder to read regexes and other code. why is this turned off?

The autofix made an error corrupting code in an older release, we can try to enable it but the scars got me cautious around this one.

18. 'no-continue': ['off'] - I think continue can usually be rewritten to a function (return) or if statements but if you prefer using continue i'm not much against that either.

For a later major, i agree, it is also under // perhaps enable in future release? for now however, continue is our bread and butter, and warnings is just too much, as is rewriting all our for/callback loops to functions in an instant.

19. 'arrow-body-style': ['off'] not a big issue but I'm not sure when it's useful to have a single long line with the return statement inside (if that was the usecase)

i think an autofix rewrite ended up creating very long chains of this, which i thought were hard to make sense of when comparing to previous look

20. 'no-cond-assign': ['off'] I agree that experienced developers should know the difference between = and ==/===, but in the years before eslint I did actually mistype this and pulling my hair out wondering why it doesn't work. Maybe we instead could useeslint-disable-next-line explicitly when we know that we want to assign inside a conditional?

maybe, yes, i think this is a candidate for a later major, i have moved it there

21. 'react/jsx-closing-tag-location': ['off'] I think that XML reads better and is easier to navigate when end tag is aligned with starting tag. If eslint --fix causes breakage, maybe better to disable the rule for the offending project or file and not disable globally in our central config eslint

The breakage caused was so significant, that i think it's not safe to use at all right now. I have moved this to next major.

22. 'import/extensions': ['warn'] maybe move this warning rule into projects that have a lot of require('xx.js')?

okay, done

23. 'no-alert': ['warn'] - alert is really not good to use in user-facing code, maybe should instead move this warn into files or projects that need to use "alert" (i'm guessing internal applications), and not have it a a global warn
24. 'no-restricted-properties': ['warn'] I understand that many projects are using deprecated globals, but maybe this rule should be disabled on those projects instead of being disabled for all other (and new) projects as well
25. 'default-case': ['warn'] not so fan of warnings as it will drown in a sea of other warnings. IMO either disable in offending projects or remove this rule completely

okay moved all these out to consuming repos

Also I see that typescript was added - why is it a necessary dependency?

i think a mistake on my part, trying to combat a peerDependency warning in the uppy repo, fixed it in master.

The rest of them lives in https://github.com/transloadit/eslint-c for review

[mifi]

here's my 2 cent:

1. dot-notation IMO this is subjective, I don't think that state['asdf'] reads better than state.asdf

I think so too, which is also the point that Evgenia made, that I imported from the content repo. I think we are in agreement and it can remain off? Or maybe i misunderstand.

Actually what I meant is that I think object.property.subproperty does read better than object['property']['subproperty']. You comment above the rule was:

    // It's true that we'd generally like to use object.property,
    // but this.state.formState is usually better read when we read
    // this.state.formState['property'].
    'dot-notation': ['off'],

I believe that is your subjective opinion, and my opinion is not very strong about tihs :)

1. no-fallthrough I'm guessing too much work to enable this

But also a question about its usefulness, i have seen legitimate cases for using fallthroughs and haven't seen a critical bug due to it yet i think 🤔

I remember having forgotten to put a break; several times between switch cases, but maybe I'm clumsy 😄 I think the thought behind the rule is that yes there are indeed legitimate use cases but then it's very easy to eslint disable next line in those cases. I don't have any strong opinion about this rule tho, as it's quite rare I use switch cases.

1. node/no-path-concat - I think this one is a good rule as it can make code work on non-unix systems too (maybe not a priority for transloadit but it's a good practice). And it can prevent accidental paths joining with double or missing slashes. e.g. '/foo/bar/' + '/file.jpg' leads to /foo/bar//file.jpg and '/foo/bar' + 'file.jpg' leads to /foo/barfile.jpg

For the time being, our focus is on linux/macos/bsd:

I think a longterm goal can be fixing it, but it is adding a whole bunch of characters to our scripts until then while they won't be windows compatible even after adding them. I think maybe for a major v5, we can turn it back on for instance

Maybe this one can be also not be turned off in our central config, but instead turned off in the projects that are using a lot of this? Because it also has the benefit of solving issues with missing or double slashes between path segments. (in new projects & new code)

1. 'no-template-curly-in-string': ['off'] I'm sure there's a good reason for disabling it, but if not then this rule could prevent accidentally using ${variable} in a normal string instead of backticked string

iirc, it would prevent us from doing '{YOUR_AUTH_KEY}', but we have a lot of those, and escaping all of them is less favorable imho

It will not prevent you from doing '{YOUR_AUTH_KEY}', it will only prevent you from doing '${YOUR_AUTH_KEY}', but if you have a lot of such custom templates in strings then I understand. But it may make sense to also only disable this rule on the projects where it is relevant.

Also rule 17,18,19,20,21 it may make sense to disable these rules in only the repos that generate errors from these rules, then the central eslint-config-transloadit can still enforce the rules for the new or unaffected projects.

[kvz]

I believe that is your subjective opinion, and my opinion is not very strong about tihs :)

I agree with you, it is not my opinion, but Evgenia's, who felt strongly that in some cases, allowing [''] is helpful I think.

I remember having forgotten to put a break; several times between switch cases, but maybe I'm clumsy I think the thought behind the rule is that yes there are indeed legitimate use cases but then it's very easy to eslint disable next line in those cases. I don't have any strong opinion about this rule tho, as it's quite rare I use switch cases.

I thought we had quite a few of these:

But maybe it's not so bad, upon first glance i can only find one in the api. I'm happy to revisit this one then for the next release 👌 (just published 1.2 before getting to this thread)

Maybe this one can be also not be turned off in our central config, but instead turned off in the projects that are using a lot of this? Because it also has the benefit of solving issues with missing or double slashes between path segments. (in new projects & new code)

oth yes, otoh, this is a transloadit standard, i don't think we are looking for others to adopt it. and so the nice thing is we can have rules in a central place, vs then have exceptions in all consuming projects. Cause i think all projects will turn this rule off. When this changes, that is the moment i agree a new release can be pushed out and exceptions in other projects can be made (?)

no-template-curly-in-string

I can agree to that reasoning yes, 1.3 then?

[kvz]

ah you are in luck, the publish failed, so they can make it in 1.2 :)