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.

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

feat: Add Eslint to sentry-javascript #2786

Merged
merged 14 commits into from
Aug 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# THIS IS A TEMPORARY FILE
# THIS WILL BE REMOVED AFTER WE FINISH ESLINT UPGRADE

packages/apm/**/*
packages/core/**/*
packages/ember/**/*
packages/gatsby/**/*
packages/hub/**/*
packages/integrations/**/*
packages/minimal/**/*
packages/node/**/*
packages/react/**/*
packages/tracing/**/*
packages/types/**/*
packages/typescript/**/*
packages/utils/**/*
127 changes: 127 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
module.exports = {
root: true,
env: {
node: true,
},
extends: ['prettier', 'eslint:recommended'],
plugins: ['sentry-sdk', 'jsdoc'],
ignorePatterns: ['eslint-plugin-sentry-sdk'],
overrides: [
{
// Configuration for JavaScript files
files: ['*.js'],
rules: {
'no-unused-vars': ['error', { argsIgnorePattern: '^_' }],
},
},
{
// Configuration for typescript files
files: ['*.ts', '*.tsx', '*.d.ts'],
extends: ['plugin:@typescript-eslint/recommended', 'prettier/@typescript-eslint'],
plugins: ['@typescript-eslint'],
parser: '@typescript-eslint/parser',
parserOptions: {
project: './tsconfig.json',
},
rules: {
// Unused variables should be removed unless they are marked with and underscore (ex. _varName).
'@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_' }],

// Make sure that all ts-ignore comments are given a description.
'@typescript-eslint/ban-ts-comment': [
'warn',
{
'ts-ignore': 'allow-with-description',
},
],

// Types usage should be explicit as possible, so we prevent usage of inferrable types.
// This is especially important because we have a public API, so usage needs to be as
// easy to understand as possible.
'@typescript-eslint/no-inferrable-types': 'off',

// Enforce type annotations to maintain consistency. This is especially important as
// we have a public API, so we want changes to be very explicit.
'@typescript-eslint/typedef': ['error', { arrowParameter: false }],
'@typescript-eslint/explicit-function-return-type': 'error',

// Consistent ordering of fields, methods and constructors for classes should be enforced
'@typescript-eslint/member-ordering': 'error',

// Enforce that unbound methods are called within an expected scope. As we frequently pass data between classes
// in SDKs, we should make sure that we are correctly preserving class scope.
'@typescript-eslint/unbound-method': 'error',

// Private and protected members of a class should be prefixed with a leading underscore
'@typescript-eslint/naming-convention': [
'error',
{
selector: 'memberLike',
modifiers: ['private'],
format: ['camelCase'],
leadingUnderscore: 'require',
},
{
selector: 'memberLike',
modifiers: ['protected'],
format: ['camelCase'],
leadingUnderscore: 'require',
},
],
},
},
{
// Configuration for files under src
files: ['src/**/*'],
rules: {
// We want to prevent async await usage in our files to prevent uncessary bundle size.
'sentry-sdk/no-async-await': 'error',

// JSDOC comments are required for classes and methods. As we have a public facing codebase, documentation,
// even if it may seems excessive at times, is important to emphasize. Turned off in tests.
'jsdoc/require-jsdoc': [
'error',
{ require: { ClassDeclaration: true, MethodDefinition: true }, checkConstructors: false },
],
},
},
{
// Configuration for test files
env: {
jest: true,
},
files: ['*.test.ts', '*.test.tsx', '*.test.js', '*.test.jsx'],
rules: {
'max-lines': 'off',
},
},
{
// Configuration for config files like webpack/rollback
files: ['*.config.js'],
parserOptions: {
sourceType: 'module',
ecmaVersion: 2018,
},
},
],

rules: {
// We want to prevent usage of unary operators outside of for loops
'no-plusplus': ['error', { allowForLoopAfterthoughts: true }],

// Disallow usage of console and alert
'no-console': 'error',
'no-alert': 'error',

// Prevent reassignment of function parameters, but still allow object properties to be
// reassigned. We want to enforce immutability when possible, but we shouldn't sacrifice
// too much efficiency
'no-param-reassign': ['error', { props: false }],

// Prefer use of template expression over string literal concatenation
'prefer-template': 'error',

// Limit maximum file size to reduce complexity. Turned off in tests.
'max-lines': 'error',
},
};
2 changes: 1 addition & 1 deletion .vscode/extensions.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
// See http://go.microsoft.com/fwlink/?LinkId=827846
// for the documentation about the extensions.json format
"recommendations": ["esbenp.prettier-vscode", "ms-vscode.vscode-typescript-tslint-plugin"]
"recommendations": ["esbenp.prettier-vscode", "ms-vscode.vscode-typescript-tslint-plugin", "dbaeumer.vscode-eslint"]
}
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
- [ember] feat: Add `@sentry/ember` (#2739)
- [apm/tracing] fix: Mark side effects for tracing hub extensions (#2788)
- [browser] ref: Use stronger function return typings (#2786)

## 5.20.1

Expand Down
74 changes: 56 additions & 18 deletions dangerfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,68 @@ import { promisify } from 'util';
import { resolve } from 'path';
import tslint from 'danger-plugin-tslint';
import { prettyResults } from 'danger-plugin-tslint/dist/prettyResults';
import { CLIEngine } from 'eslint';

const packages = ['apm', 'browser', 'core', 'hub', 'integrations', 'minimal', 'node', 'types', 'utils'];
const PACKAGES = ['apm', 'core', 'hub', 'integrations', 'minimal', 'node', 'types', 'utils'];
const EXTENSIONS = ['.js', '.jsx', '.ts', '.tsx'];

export default async () => {
/**
* Eslint your code with Danger
* Based on fork from: https://github.com/appcelerator/danger-plugin-eslint
*/
async function eslint(): Promise<void[]> {
const allFiles = danger.git.created_files.concat(danger.git.modified_files);
const cli = new CLIEngine({});
// let eslint filter down to non-ignored, matching the extensions expected
const filesToLint = allFiles.filter(f => !cli.isPathIgnored(f) && EXTENSIONS.some(ext => f.endsWith(ext)));
return Promise.all(filesToLint.map(f => lintFile(cli, f)));
}

/** JSDoc */
async function lintFile(linter: CLIEngine, path: string): Promise<void> {
const contents = await danger.github.utils.fileContents(path);
const report = linter.executeOnText(contents, path);

if (report.results.length !== 0) {
report.results[0].messages.map(msg => {
if (msg.fatal) {
fail(`Fatal error linting ${path} with eslint.`);
return;
}

const noop = (): void => undefined;
const fn = { 0: noop, 1: warn, 2: fail }[msg.severity];

fn(`${path} line ${msg.line} – ${msg.message} (${msg.ruleId})`, path, msg.line);
});
}
}

export default async (): Promise<void> => {
if (!danger.github) {
return;
}

schedule(async () => {
const tsLintResult = (await Promise.all(
packages.map(packageName => {
return new Promise<string>(res => {
tslint({
lintResultsJsonPath: resolve(__dirname, 'packages', packageName, 'lint-results.json'),
handleResults: results => {
if (results.length > 0) {
const formattedResults = prettyResults(results);
res(`TSLint failed: **@sentry/${packageName}**\n\n${formattedResults}`);
} else {
res('');
}
},
const tsLintResult = (
await Promise.all(
PACKAGES.map(packageName => {
return new Promise<string>(res => {
tslint({
lintResultsJsonPath: resolve(__dirname, 'packages', packageName, 'lint-results.json'),
handleResults: results => {
if (results.length > 0) {
const formattedResults = prettyResults(results);
res(`TSLint failed: **@sentry/${packageName}**\n\n${formattedResults}`);
} else {
res('');
}
},
});
});
});
}),
)).filter(str => str.length);
}),
)
).filter(str => str.length);
if (tsLintResult.length) {
tsLintResult.forEach(tsLintFail => {
fail(`${tsLintFail}`);
Expand All @@ -39,6 +75,8 @@ export default async () => {
}
});

await eslint();

const hasChangelog = danger.git.modified_files.indexOf('CHANGELOG.md') !== -1;
const isTrivial = (danger.github.pr.body + danger.github.pr.title).includes('#trivial');

Expand Down
50 changes: 50 additions & 0 deletions eslint-plugin-sentry-sdk/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// This is a temporary file. It will be removed when we migrate
// the eslint configs to another repo.

'use strict';

/**
* Rule to disallow usage of async await
* @author Abhijeet Prasad
*/
const noAsyncAwait = {
meta: {
type: 'problem',
docs: {
description: 'Disallow usage of async await',
category: 'Best Practices',
recommended: true,
},
fixable: null,
schema: [],
},
create: function(context) {
return {
FunctionDeclaration(node) {
if (node.async) {
context.report({
node,
message:
'Using async-await can add a lot to bundle size. Please do not use it outside of tests, use Promises instead',
});
}
},

ArrowFunctionExpression(node) {
if (node.async) {
context.report({
node,
message:
'Using async-await can add a lot to bundle size. Please do not use it outside of tests, use Promises instead',
});
}
},
};
},
};

module.exports = {
rules: {
'no-async-await': noAsyncAwait,
},
};
11 changes: 11 additions & 0 deletions eslint-plugin-sentry-sdk/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "eslint-plugin-sentry-sdk",
"version": "0.0.1",
"main": "index.js",
"devDependencies": {
"eslint": "^7.5.0"
},
"engines": {
"node": ">=0.10.0"
}
}
7 changes: 7 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,17 @@
"@types/mocha": "^5.2.0",
"@types/node": "^11.13.7",
"@types/sinon": "^7.0.11",
"@typescript-eslint/eslint-plugin": "^3.7.1",
"@typescript-eslint/parser": "^3.7.1",
"chai": "^4.1.2",
"codecov": "^3.6.5",
"danger": "^7.1.3",
"danger-plugin-eslint": "^0.1.0",
"danger-plugin-tslint": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because the other packages still use tslint. We can remove after we are done.

"eslint": "^7.5.0",
"eslint-config-prettier": "^6.11.0",
"eslint-plugin-jsdoc": "^30.0.3",
"eslint-plugin-sentry-sdk": "file:./eslint-plugin-sentry-sdk",
Copy link
Member Author

Choose a reason for hiding this comment

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

All of these dependencies are temporary. When we move the main eslint config out of this repo, we should be able to replace this (at the package level) with just a single eslint-config-sentry-sdks

"jest": "^24.7.1",
"karma-browserstack-launcher": "^1.5.1",
"karma-firefox-launcher": "^1.1.0",
Expand Down
47 changes: 47 additions & 0 deletions packages/browser/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
module.exports = {
root: true,
env: {
es6: true,
browser: true,
},
parserOptions: {
ecmaVersion: 2018,
},
extends: ['../../.eslintrc.js'],
ignorePatterns: ['build/**/*', 'dist/**/*', 'esm/**/*', 'examples/**/*', 'scripts/**/*', 'src/loader.js'],
overrides: [
{
files: ['*.ts', '*.tsx', '*.d.ts'],
parserOptions: {
project: './tsconfig.json',
},
},
{
files: ['test/**/*'],
rules: {
'jsdoc/require-jsdoc': 'off',
'no-console': 'off',
'max-lines': 'off',
'prefer-template': 'off',
},
},
{
files: ['test/integration/**/*'],
env: {
mocha: true,
},
rules: {
'no-undef': 'off',
},
},
{
files: ['test/integration/common/**/*', 'test/integration/suites/**/*'],
rules: {
'no-unused-vars': 'off',
},
},
],
rules: {
'no-prototype-builtins': 'off',
},
};
Loading