Tweak eslint settings

[terencehonles]

Description of the change

Update the eslint configuration so it is more easy to understand and make sure eslint is run before testing so testing doesn't hang.

See: https://github.com/rollbar/rollbar.js/actions/runs/488638525 took 5 hours 😒

Description here

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers mentioned in a comment
• Changes have been reviewed by at least one other engineer
• Issue from task tracker has a link to this pull request

[waltjones]

I'd much rather have a complexity threshold we expect to meet, than to have eslint-disable-next-line complexity sprinkled everywhere.

[waltjones]

What's the rationale for moving these?

[terencehonles]

This is why no-use-before-define is currently 0 (off). They don't seem to be referenced anywhere else and this seems better than adding eslint-disable-next-line and leaving this disabled.

[waltjones]

My narrow concern with this is there are two other entry points with this same pattern that should be kept organized uniformly.
https://github.com/rollbar/rollbar.js/blob/master/src/server/rollbar.js
https://github.com/rollbar/rollbar.js/blob/master/src/react-native/rollbar.js

I'm noticing they both avoid the lint error by assigning Rollbar.defaultOptions. Maybe that's preferable here as well. Either way, the layout of each should be predictable when navigating between them.

[terencehonles]

I can change it to do that instead, but it may make sense to just change those all to be like this too. When looking into this comment I noticed

rollbar.js/src/api.js

Lines 4 to 11 in 600c425

	var defaultOptions = {
	hostname: 'api.rollbar.com',
	path: '/api/1/item/',
	search: null,
	version: '1',
	protocol: 'https:',
	port: 443
	};

has the defaultOptions at the top. The main difference would be that if it's attached to the Rollbar object people may be touching it and the browser version does not have the ability to access the defaultOptions (for better or worse)

[terencehonles]

I'd much rather have a complexity threshold we expect to meet, than to have eslint-disable-next-line complexity sprinkled everywhere.

Was 35 chosen for a specific reason? Does it make sense to apply to the whole codebase? I lowered it to 20 from 35 so more of the codebase is required to be less complex. There are a few offenders (1 which had a eslint-disable-next-line already, but it may not have been needed with 35).

Instead of setting the limit so high it might make sense to know where the highly complex functions are and work on reducing their complexity. I was not familiar with the rule, and there's no comment in the .eslintrc or its history so I don't know why a function I added an || to now is "too complex". When removing the rule it pointed to only a few offenders which look like they probably should be fixed (the last paragraph). I can look at fixing some of those, but I hope this explanation highlights that a high limit of 35 will cause functions to reach a very high level of complexity and using the default may be better to prod breaking down functions earlier.

[waltjones]

Your PR is the only thing that exceeded the previous threshold. The changes you made allow all of the same existing code as before, except with special case eslint comments throughout. And your PR ends up being the only thing that exceeds the previous threshold. If you lower the complexity of your PR to the threshold that was set, none of the other changes are needed.

[terencehonles]

Your PR is the only thing that exceeded the previous threshold. The changes you made allow all of the same existing code as before, except with special case eslint comments throughout. And your PR ends up being the only thing that exceeds the previous threshold. If you lower the complexity of your PR to the threshold that was set, none of the other changes are needed.

I think we may be getting off on the wrong foot. Of course I know my changes triggered the lint warning. I'm just asking and also suggesting that the previous complexity may have been set somewhat arbitrarily. You may not agree and if you really don't want to lower the complexity that's fine. I don't have to lower the complexity to 20 from 35. I'm just changing a lint rule here.

How I handle #915 is a separate question and as you point out it can be extracted into a function. I believe my point still stands, it makes sense to address complexity earlier rather than waiting to hit the magical number of 35. Why is 20 less magical? I guess it's not really, but it's at least the default that eslint provides 🤷 .

[waltjones]

The numbers are arbitrary, but it isn't arbitrary when code is refactored for less complexity. If you proposed to set it, for example, to 30 or 25 and then refactor the code to be compliant, I'd be in favor of that. But setting it lower and then overriding all the violations (and no plan to refactor the relevant code), nothing has been improved.

In this case the rule was lowered in this PR to 20, but in the related #915 the code exceeds the previous limit of 35. The two PRs taken as a whole (a) set the limit so low that there are many overrides and the limit is essentially not enforced, and (b) add code that breaks the previous higher limit.

[terencehonles]

The numbers are arbitrary, but it isn't arbitrary when code is refactored for less complexity. If you proposed to set it, for example, to 30 or 25 and then refactor the code to be compliant, I'd be in favor of that. But setting it lower and then overriding all the violations (and no plan to refactor the relevant code), nothing has been improved.

I agree "nothing has been improved", but the argument I'm making is it actually gives more insight into the problem in the long run. As is, the complexity of the codebase could creep up to 35, but setting the complexity back to the default would check all the code and it could be required that no new eslint-disable lines are added and that future changes should be encouraged to remove existing eslint-disable lines when possible. Since it's a small number of changes I don't mind potentially taking a stab at a few of them (remember I'm a volunteer), but what I'm describing is a progressive improvement to the code rather than always requiring a large PR which a contributor may not be willing to help with.

In this case the rule was lowered in this PR to 20, but in the related #915 the code exceeds the previous limit of 35. The two PRs taken as a whole (a) set the limit so low that there are many overrides and the limit is essentially not enforced, and (b) add code that breaks the previous higher limit.

This is why I split the two PRs out. #915 no longer breaks the existing limit of 35 and it can be reviewed and merged independently of this PR. In the other PR I do bring up that the use of typeName may start to become problematic, but I believe the code I contributed does not contribute to the problem and the potential typeName change can be tracked be tracked internally and/or fixed when it becomes a problem for someone (I would hope it would not be me because I would appreciate the problem addressed before I have to spend time debugging the library which is how I came upon the issue for #915 )

[waltjones]

@terencehonles A few of the other Rollbar repos have recently enabled Discussions, and I have just enabled it for Rollbar.js: https://github.com/rollbar/rollbar.js/discussions/

For policy discussions (like changes to lint rules), this might be a way to reach a consensus before opening a PR, might be more discoverable by others interested in the topic, and the various Rollbar SDK maintainers are hoping the format is better for some things than using Issues.

That said, feel free to post your feedback here or there.

Re. your last comment above:

progressive improvement to the code rather than always requiring a large PR

I'm also in favor of progressive improvement and would prefer to see the threshold lowered in step with the code being refactored. Maybe setting complexity = 30 requires light refactoring of 1 or 2 functions and it's not such a big PR.

[terencehonles]

I've bumped it down to 32 since that's actually where createItem appears to be and removed an existing offender using eslint-disable-next-line complexity. I believe at this point I've removed any changes that might be controversial and any future changes to the complexity would have this conversation referenced in the PR and future efforts can be done to lower the complexity (possibly the eventual change in #915 will allow lowering it but Instrumenter.prototype.instrument is still at 31 even with the changes I made so that might prevent dropping much lower).