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

chore: type checking in one place makes bundle smaller #843

Merged
merged 8 commits into from
Oct 24, 2023
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
19 changes: 19 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const extend = [
'plugin:react-hooks/recommended',
'prettier',
'plugin:compat/recommended',
'plugin:posthog-js/all',
]

module.exports = {
Expand All @@ -44,6 +45,12 @@ module.exports = {
react: {
version: '17.0',
},
'import/resolver': {
node: {
paths: ['eslint-rules'], // Add the directory containing your custom rules
extensions: ['.js', '.jsx', '.ts', '.tsx'], // Ensure ESLint resolves both JS and TS files
},
},
},
overrides: [
{
Expand All @@ -57,6 +64,18 @@ module.exports = {
'no-console': 'off',
},
},
{
files: 'eslint-rules/**/*',
extends: ['eslint:recommended', 'prettier'],
rules: {
'prettier/prettier': 'error',
'@typescript-eslint/no-var-requires': 'off',
'posthog-js/no-direct-null-check': 'off',
},
env: {
node: true,
},
},
],
root: true,
}
8 changes: 8 additions & 0 deletions eslint-rules/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# PostHog JS Custom ESLint rules

This package contains custom ESLint rules for PostHog's JS codebase.

For example, we have a number of functions that check types like `_isNull` or `isBoolean`.
In most projects these don't help very much but since posthog-js is bundled and included in many different projects,
we want to ensure the bundle size is as small as possible. Moving to these functions reduced bundle by 1%, so we
use a set of custom linters to ensure we don't accidentally add new code that does not use these helpers.
124 changes: 124 additions & 0 deletions eslint-rules/custom-eslint-rules.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
const noDirectNullCheck = require('./no-direct-null-check')
const noDirectUndefinedCheck = require('./no-direct-undefined-check')
const noDirectArrayCheck = require('./no-direct-array-check')
const noDirectIsFunctionCheck = require('./no-direct-function-check')
const noDirectObjectCheck = require('./no-direct-object-check')
const noDirectStringCheck = require('./no-direct-string-check')
const noDirectDateCheck = require('./no-direct-date-check')
const noDirectNumberCheck = require('./no-direct-number-check')
const noDirectBooleanCheck = require('./no-direct-boolean-check')

const { RuleTester } = require('eslint')

const ruleTester = new RuleTester()

ruleTester.run('no-direct-null-check', noDirectNullCheck, {
valid: [{ code: `_isNull(x)` }],
invalid: [{ code: `x === null`, errors: [{ message: 'Use _isNull() instead of direct null checks.' }] }],
})

ruleTester.run('no-direct-undefined-check', noDirectUndefinedCheck, {
valid: [{ code: `_isUndefined(x)` }],
invalid: [
{
code: `typeof x === undefined`,
errors: [{ message: 'Use _isUndefined() instead of direct undefined checks.' }],
},
],
})

ruleTester.run('no-direct-array-check', noDirectArrayCheck, {
valid: [{ code: `_isArray(x)` }],
invalid: [
{
code: `Array.isArray(x)`,
errors: [{ message: 'Use _isArray() instead of direct array checks.' }],
},
],
})

ruleTester.run('no-direct-is-function-check', noDirectIsFunctionCheck, {
valid: [{ code: `_isFunction(x)` }],
invalid: [
{
code: `/^\\s*\\bfunction\\b/.test(x)`,
errors: [{ message: 'Do not use regex to check for functions. Use _isFunction instead.' }],
},
{
code: `x instanceof Function`,
errors: [{ message: "Do not use 'instanceof Function' to check for functions. Use _isFunction instead." }],
},
{
code: `typeof x === "function"`,
errors: [
{ message: 'Do not use \'typeof x === "function"\' to check for functions. Use _isFunction instead.' },
],
},
],
})

ruleTester.run('no-direct-object-check', noDirectObjectCheck, {
valid: [{ code: `_isObject(x)` }],
invalid: [
{
code: `obj === Object(obj)`,
errors: [{ message: "Do not use 'obj === Object(obj)'. Use _isObject instead." }],
},
],
})

ruleTester.run('no-direct-string-check', noDirectStringCheck, {
valid: [{ code: `_isString(x)` }],
invalid: [
{
code: `toString.call(x) == '[object String]'`,
errors: [{ message: 'Use _isString instead of direct string checks.' }],
},
{
code: `x instanceof String`,
errors: [{ message: 'Use _isString instead of direct string checks.' }],
},
],
})

ruleTester.run('no-direct-date-check', noDirectDateCheck, {
valid: [{ code: `_isDate(x)` }],
invalid: [
{
code: `toString.call(obj) == '[object Date]'`,
errors: [{ message: 'Use _isDate instead of direct date checks.' }],
},
{
code: `x instanceof Date`,
errors: [{ message: 'Use _isDate instead of direct date checks.' }],
},
],
})

ruleTester.run('no-direct-number-check', noDirectNumberCheck, {
valid: [{ code: `_isNumber(x)` }],
invalid: [
{
code: `toString.call(obj) == '[object Number]'`,
errors: [{ message: 'Use _isNumber instead of direct number checks.' }],
},
{
code: `typeof x === 'number'`,
errors: [{ message: 'Use _isNumber instead of direct number checks.' }],
},
],
})

ruleTester.run('no-direct-boolean-check', noDirectBooleanCheck, {
valid: [{ code: `_isBoolean(x)` }],
invalid: [
{
code: `toString.call(obj) == '[object Boolean]'`,
errors: [{ message: 'Use _isBoolean instead of direct boolean checks.' }],
},
{
code: `typeof x === 'boolean'`,
errors: [{ message: 'Use _isBoolean instead of direct boolean checks.' }],
},
],
})
17 changes: 17 additions & 0 deletions eslint-rules/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const { readdirSync } = require('fs')
const { basename } = require('path')

const projectName = 'posthog-js'
const ruleFiles = readdirSync('eslint-rules').filter(
(file) => file.endsWith('.js') && file !== 'index.js' && !file.endsWith('test.js')
)
const configs = {
all: {
plugins: [projectName],
rules: Object.fromEntries(ruleFiles.map((file) => [`${projectName}/${basename(file, '.js')}`, 'error'])),
},
}

const rules = Object.fromEntries(ruleFiles.map((file) => [basename(file, '.js'), require('./' + file)]))

module.exports = { configs, rules }
20 changes: 20 additions & 0 deletions eslint-rules/no-direct-array-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module.exports = {
create(context) {
return {
MemberExpression: function (node) {
// Check if the object is 'Array' and the property is 'isArray'
if (
node.object.type === 'Identifier' &&
node.object.name === 'Array' &&
node.property.type === 'Identifier' &&
node.property.name === 'isArray'
) {
context.report({
node,
message: 'Use _isArray() instead of direct array checks.',
})
}
},
}
},
}
38 changes: 38 additions & 0 deletions eslint-rules/no-direct-boolean-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
module.exports = {
create(context) {
return {
BinaryExpression: function (node) {
// Check for `toString.call(obj) == '[object Boolean]'`
if (
(node.operator === '==' || node.operator === '===') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

So meta 😂

node.left.type === 'CallExpression' &&
node.left.callee.property &&
node.left.callee.property.name === 'call' &&
node.left.callee.object &&
node.left.callee.object.name === 'toString' &&
node.right.type === 'Literal' &&
node.right.value === '[object Boolean]'
) {
context.report({
node,
message: 'Use _isBoolean instead of direct boolean checks.',
})
}

// Check for `typeof x === 'boolean'`
if (
node.operator === '===' &&
node.left.type === 'UnaryExpression' &&
node.left.operator === 'typeof' &&
node.right.type === 'Literal' &&
node.right.value === 'boolean'
) {
context.report({
node,
message: 'Use _isBoolean instead of direct boolean checks.',
})
}
},
}
},
}
32 changes: 32 additions & 0 deletions eslint-rules/no-direct-date-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
module.exports = {
create(context) {
return {
BinaryExpression: function (node) {
// Check for `toString.call(obj) == '[object Date]'`
if (
(node.operator === '==' || node.operator === '===') &&
node.left.type === 'CallExpression' &&
node.left.callee.property &&
node.left.callee.property.name === 'call' &&
node.left.callee.object &&
node.left.callee.object.name === 'toString' &&
node.right.type === 'Literal' &&
node.right.value === '[object Date]'
) {
context.report({
node,
message: 'Use _isDate instead of direct date checks.',
})
}

// Check for `x instanceof Date`
if (node.operator === 'instanceof' && node.right.type === 'Identifier' && node.right.name === 'Date') {
context.report({
node,
message: 'Use _isDate instead of direct date checks.',
})
}
},
}
},
}
52 changes: 52 additions & 0 deletions eslint-rules/no-direct-function-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
module.exports = {
create(context) {
return {
CallExpression: function (node) {
// Check if the callee object is a regex matching function pattern
const isFunctionPatternRegex =
node.callee.type === 'MemberExpression' &&
node.callee.object?.type === 'Literal' &&
node.callee.object?.regex &&
node.callee.object?.regex.pattern === '^\\s*\\bfunction\\b'

// Check if the callee property is 'test'
const isTestCall = node.callee.property?.type === 'Identifier' && node.callee.property?.name === 'test'

if (isFunctionPatternRegex && isTestCall) {
context.report({
node,
message: 'Do not use regex to check for functions. Use _isFunction instead.',
})
}
},
BinaryExpression: function (node) {
// Check if the operator is 'instanceof' and the right operand is 'Function'
if (
node.operator === 'instanceof' &&
node.right.type === 'Identifier' &&
node.right.name === 'Function'
) {
context.report({
node,
message: "Do not use 'instanceof Function' to check for functions. Use _isFunction instead.",
})
}

// Check for 'typeof x === "function"' pattern
if (
node.operator === '===' &&
node.left.type === 'UnaryExpression' &&
node.left.operator === 'typeof' &&
node.right.type === 'Literal' &&
node.right.value === 'function'
) {
context.report({
node,
message:
'Do not use \'typeof x === "function"\' to check for functions. Use _isFunction instead.',
})
}
},
}
},
}
21 changes: 21 additions & 0 deletions eslint-rules/no-direct-null-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
module.exports = {
create(context) {
return {
BinaryExpression: function (node) {
// Check if either the left or right operand is a null literal
if (
(node.left.type === 'Literal' && node.left.value === null) ||
(node.right.type === 'Literal' && node.right.value === null)
) {
// Check the operator to ensure it's a comparison (you can expand this list if needed)
if (node.operator === '===' || node.operator === '!==') {
context.report({
node,
message: 'Use _isNull() instead of direct null checks.',
})
}
}
},
}
},
}
38 changes: 38 additions & 0 deletions eslint-rules/no-direct-number-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
module.exports = {
create(context) {
return {
BinaryExpression: function (node) {
// Check for `toString.call(obj) == '[object Number]'`
if (
(node.operator === '==' || node.operator === '===') &&
node.left.type === 'CallExpression' &&
node.left.callee.property &&
node.left.callee.property.name === 'call' &&
node.left.callee.object &&
node.left.callee.object.name === 'toString' &&
node.right.type === 'Literal' &&
node.right.value === '[object Number]'
) {
context.report({
node,
message: 'Use _isNumber instead of direct number checks.',
})
}

// Check for `typeof x === 'number'`
if (
node.operator === '===' &&
node.left.type === 'UnaryExpression' &&
node.left.operator === 'typeof' &&
node.right.type === 'Literal' &&
node.right.value === 'number'
) {
context.report({
node,
message: 'Use _isNumber instead of direct number checks.',
})
}
},
}
},
}
Loading
Loading