Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak eslint settings #916

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Tweak eslint settings #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
001265c
tweak eslint settings
terencehonles Jan 15, 2021
09ff2af
fix and re-enable eslint no-use-before-define
terencehonles Jan 16, 2021
8ada628
update GitHub workflow to lint before testing so lint errors don't hang
terencehonles Jan 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
],
"globals": { "Promise": true },
"rules": {
"strict": 0,
"camelcase": [2, {"properties": "never"}],
"quotes": [2, "single", "avoid-escape"],
"no-underscore-dangle": 0,
"no-useless-escape": 0,
"complexity": [2, { "max": 35 }],
"no-use-before-define": [0, { "functions": false }],
"no-unused-vars": [2, { "argsIgnorePattern": "^_" }],
"no-prototype-builtins": 0
"strict": "off",
"camelcase": ["error", {"properties": "never"}],
"quotes": ["error", "single", "avoid-escape"],
"no-underscore-dangle": "off",
"no-useless-escape": "off",
"complexity": ["error", {"max": 32}],
"no-use-before-define": ["error", { "functions": false }],
"no-unused-vars": ["error", { "argsIgnorePattern": "^_" }],
"no-prototype-builtins": "off"
}
}
4 changes: 4 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ jobs:
matrix:
node-version: [8, 10, 12]

name: Test / Node ${{ matrix.node-version }}
steps:
- uses: actions/checkout@v2
with:
Expand All @@ -27,5 +28,8 @@ jobs:
- name: npm install
run: npm install

- name: Lint
run: npm run lint

- name: Run tests
run: npm run test_ci
22 changes: 3 additions & 19 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,8 @@
TESTS = test/index.html
REPORTER = dot

build:
@npm run build
build lint test test-browser test-server test_ci:
@npm run $@
@echo ""

test:
@npm run test
@echo ""

test-browser:
@npm run test-browser
@echo ""

test-server:
@npm run test-server
@echo ""

test_ci:
@npm run test_ci
@echo ""

.PHONY: test test_ci
.PHONY: build lint test test-browser test-server test_ci
42 changes: 21 additions & 21 deletions src/browser/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,27 @@ var predicates = require('./predicates');
var sharedPredicates = require('../predicates');
var errorParser = require('../errorParser');

var defaults = require('../defaults');
var scrubFields = require('./defaults/scrubFields');

var defaultOptions = {
version: defaults.version,
scrubFields: scrubFields.scrubFields,
logLevel: defaults.logLevel,
reportLevel: defaults.reportLevel,
uncaughtErrorLevel: defaults.uncaughtErrorLevel,
endpoint: defaults.endpoint,
verbose: false,
enabled: true,
transmit: true,
sendConfig: false,
includeItemsInTelemetry: true,
captureIp: true,
inspectAnonymousErrors: true,
ignoreDuplicateErrors: true,
wrapGlobalEventHandlers: false
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for moving these?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


function Rollbar(options, client) {
this.options = _.handleOptions(defaultOptions, options, null, logger);
this.options._configuredOptions = options;
Expand Down Expand Up @@ -537,25 +558,4 @@ function _gWindow() {
return ((typeof window != 'undefined') && window) || ((typeof self != 'undefined') && self);
}

var defaults = require('../defaults');
var scrubFields = require('./defaults/scrubFields');

var defaultOptions = {
version: defaults.version,
scrubFields: scrubFields.scrubFields,
logLevel: defaults.logLevel,
reportLevel: defaults.reportLevel,
uncaughtErrorLevel: defaults.uncaughtErrorLevel,
endpoint: defaults.endpoint,
verbose: false,
enabled: true,
transmit: true,
sendConfig: false,
includeItemsInTelemetry: true,
captureIp: true,
inspectAnonymousErrors: true,
ignoreDuplicateErrors: true,
wrapGlobalEventHandlers: false
};

module.exports = Rollbar;
3 changes: 1 addition & 2 deletions src/browser/shim.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ function _wrapInternalErr(f) {
return f.apply(this, arguments);
} catch (e) {
try {
/* eslint-disable no-console */
/* eslint-disable-next-line no-console */
console.error('[Rollbar]: Internal error', e);
/* eslint-enable no-console */
} catch (e2) {
// Ignore
}
Expand Down
36 changes: 19 additions & 17 deletions src/browser/telemetry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,41 +120,46 @@ Instrumenter.prototype.configure = function(options) {
}
};

// eslint-disable-next-line complexity
Instrumenter.prototype.instrument = function(oldSettings) {
if (this.autoInstrument.network && !(oldSettings && oldSettings.network)) {
var network = oldSettings && oldSettings.network;
if (this.autoInstrument.network && !network) {
this.instrumentNetwork();
} else if (!this.autoInstrument.network && oldSettings && oldSettings.network) {
} else if (!this.autoInstrument.network && network) {
this.deinstrumentNetwork();
}

if (this.autoInstrument.log && !(oldSettings && oldSettings.log)) {
var log = oldSettings && oldSettings.log;
if (this.autoInstrument.log && !log) {
this.instrumentConsole();
} else if (!this.autoInstrument.log && oldSettings && oldSettings.log) {
} else if (!this.autoInstrument.log && log) {
this.deinstrumentConsole();
}

if (this.autoInstrument.dom && !(oldSettings && oldSettings.dom)) {
var dom = oldSettings && oldSettings.dom;
if (this.autoInstrument.dom && !dom) {
this.instrumentDom();
} else if (!this.autoInstrument.dom && oldSettings && oldSettings.dom) {
} else if (!this.autoInstrument.dom && dom) {
this.deinstrumentDom();
}

if (this.autoInstrument.navigation && !(oldSettings && oldSettings.navigation)) {
var navigation = oldSettings && oldSettings.navigation;
if (this.autoInstrument.navigation && !navigation) {
this.instrumentNavigation();
} else if (!this.autoInstrument.navigation && oldSettings && oldSettings.navigation) {
} else if (!this.autoInstrument.navigation && navigation) {
this.deinstrumentNavigation();
}

if (this.autoInstrument.connectivity && !(oldSettings && oldSettings.connectivity)) {
var connectivity = oldSettings && oldSettings.connectivity;
if (this.autoInstrument.connectivity && !connectivity) {
this.instrumentConnectivity();
} else if (!this.autoInstrument.connectivity && oldSettings && oldSettings.connectivity) {
} else if (!this.autoInstrument.connectivity && connectivity) {
this.deinstrumentConnectivity();
}

if (this.autoInstrument.contentSecurityPolicy && !(oldSettings && oldSettings.contentSecurityPolicy)) {
var contentSecurityPolicy = oldSettings && oldSettings.contentSecurityPolicy;
if (this.autoInstrument.contentSecurityPolicy && !contentSecurityPolicy) {
this.instrumentContentSecurityPolicy();
} else if (!this.autoInstrument.contentSecurityPolicy && oldSettings && oldSettings.contentSecurityPolicy) {
} else if (!this.autoInstrument.contentSecurityPolicy && contentSecurityPolicy) {
this.deinstrumentContentSecurityPolicy();
}
};
Expand Down Expand Up @@ -222,9 +227,7 @@ Instrumenter.prototype.instrumentNetwork = function() {
}, this.replacements, 'network');

replace(xhrp, 'send', function(orig) {
/* eslint-disable no-unused-vars */
return function(data) {
/* eslint-enable no-unused-vars */
var xhr = this;

function onreadystatechangeHandler() {
Expand Down Expand Up @@ -331,9 +334,8 @@ Instrumenter.prototype.instrumentNetwork = function() {

if ('fetch' in this._window) {
replace(this._window, 'fetch', function(orig) {
/* eslint-disable no-unused-vars */
/* eslint-disable-next-line no-unused-vars */
return function(fn, t) {
/* eslint-enable no-unused-vars */
var args = new Array(arguments.length);
for (var i=0, len=args.length; i < len; i++) {
args[i] = arguments[i];
Expand Down