chore: update @silvermine/eslint-config #105

MatheusBaldi · 2025-01-07T15:34:49Z

When not eslint specific, the comments are questions or notes I've used to remember things during implementation. After the review I'll remove them.

This PR replaces this one created by renovate: #96

MrMarCode · 2025-01-10T18:26:27Z

src/types/KeyValueStringObject.ts

   // Arrays and the array-like `arguments` variable are objects, so they would not be
   // caught by an `isObject` check
   if (!isObject(o) || isArray(o) || isArguments(o)) {
      return false;
   }

-   for (let k of Object.keys(o)) {
-      let v: any = (o as any)[k];
+   // Object.keys returns an array of strings


Suggested change

// Object.keys returns an array of strings

This is already explained in the docstring for Object.keys and by typescripts hints.

MrMarCode · 2025-01-10T18:28:46Z

tests/types/RequireDefined.test.ts

Admittedly this test does seem useless, but I think deleting it is out of scope for this task. Can we instead add a comment here to disable no-empty-function please similar to how you solved it in the other cases?

MrMarCode · 2025-01-10T19:03:52Z

src/types/StringArrayOfStringsMap.ts

   if (!isObject(o) || isArray(o) || isArguments(o)) {
      return false;
   }

   for (const k of Object.keys(o)) {
-      if (!isArrayOfStrings((o as any)[k])) {
+      if (!isArrayOfStrings((o as Record<string, unknown>)[k])) {


Try using our existing type guard in all the TS files that rely on a similar check:

if (!isStringUnknownMap(o)) { return false; } for (const k of Object.keys(o)) { if (!isArrayOfStrings(o[k])) {

We prefer type guards over the as keyword because type guards actually run code to validate the type before asserting that it is that type. However, the as keyword has no checks. Of course, our original goal was to have as little impact on the code as possible, but since this change is small and we get to use type guards instead of as I think it's worth it.

MrMarCode · 2025-01-10T19:11:38Z

src/types/StringMap.ts

   // Arrays and the array-like `arguments` variable are objects, so they would not be
   // caught by an `isObject` check
   if (!isObject(o) || isArray(o) || isArguments(o)) {
      return false;
   }

   for (const k of Object.keys(o)) {
-      if (!isString((o as any)[k])) {
+      if (!isString((o as Record<string, unknown>)[k])) {


(See comment regarding type guards)

MrMarCode · 2025-01-10T19:25:31Z

src/utils/is-empty.ts

@@ -12,7 +12,7 @@ import { isUndefined } from './is-undefined';
 *
 * @returns `true` if `o` is empty
 */
-export function isEmpty(o: any): boolean {
+export function isEmpty(o: unknown): boolean {


This function now throws an error when running the tests.

This has happened to me many times. It's usually because I test it, make another change, then forget to test it again before submitting for review. Here are a few tips that help me avoid this problem:

The developer who wrote or modified the code is responsible to exercise that code before submitting it. Try creating a checklist of tasks to do before submitting for review and include 'create a test plan' in that checklist.

Make sure to test the output of the code, not just it's functionality. It may run without errors, but does it produce the correct output? (This is not a problem here, because our automated tests check this for us. But it's good to keep in mind for future tests.)

For each PR I submit, I try to include a comment describing how I tested the code (even if it's as simple as 'ran the automated tests'). This serves as a reminder for me, and it's helpful when I need to re-test things after a review comes back with requested changes.

I hope that helps! Unfortunately I learned these lessons the hard way. Next time we chat I can tell you a sad story about how I submitted broken code to production 😬

MrMarCode · 2025-01-10T19:34:01Z

src/utils/is-map-with-values-of-type.ts

   if (!isStringUnknownMap(o)) {
      return false;
   }

   for (const k of Object.keys(o)) {
-      if (!guard((o as any)[k])) {
+      if (!guard((o as Record<string, unknown>)[k])) {


Type coercion is not necessary here because of the type guard used earlier in the function

if (!guard(o[k])) {

MrMarCode · 2025-01-10T19:51:09Z

src/utils/is-promise-like.ts

+   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   return isPromise(o) || (isObject(o) && typeof (o as any).then === 'function');


Can we get more specific with this type so that we don't have to disable the rule? E.g:

return isPromise(o) || (isObject(o) && typeof (o as PromiseLike<unknown>).then === 'function');

@MrMarCode I thought about it, but then we are assuming we already have the right type before checking it. In practice it doesn't make a difference regarding execution time, but I thought it could confuse developers who read this code in the future. By disabling the rule, the developer clearly sees that the object can be anything at that point of the code; by assuming a PromiseLike type, even though the object can be anything, it may lead the developer to a different conclusion. I have no problem with this change, but I want to know your opinion before applying it.

I can see how it could be confusing for a future developer, but I think it should be clear enough for at least 2 reasons:

The function's comment explains that it "returns true if o is Promise-like (i.e. has a then function)".

Developers should probably assume that the types inside a type guard are not valid until the function returns true, since that's the purpose of a type guard. If they start with that assumption, it should avoid the confusion.

MrMarCode · 2025-01-10T20:02:48Z

tests/utils/pick.test.ts

+      // eslint-disable-next-line no-proto, @typescript-eslint/no-explicit-any
+      (objWithProto as any).__proto__ = { b: 2 };


Can we adjust the code to address the deprecation flag instead of disabling the rule? (docs)
Maybe something like this:

Object.setPrototypeOf(objWithProto, { b: 2 });

MrMarCode · 2025-01-10T20:20:02Z

src/utils/get.ts

When not eslint specific, the comments are questions or notes I've used to remember things during implementation. After the review I'll remove them.

This PR replaces this one created by renovate: #96

Nice job on the notes. It's very helpful for keeping track of things as you implement changes. The only thing I would suggest is if you could not include any personal notes in code you submit for review. Maybe try using git's stash feature in order to separate out your notes before committing, or you could even write them in a separate program (I use LogSeq for this purpose because it supports writing code blocks and todo lists). Whatever works best for you 👍. But having the notes in the code review makes it difficult on the reviewer to understand what's going on and what code is considered 'done'. I've skipped reviewing this file and I can finish the review once the personal notes are removed and the other comments are addressed.

Thanks for all your hard work on this Matheus! It is much appreciated! 🙂

Thank for the tip. I use the notes from xfce environment. These ones were added here for another pupose.

MrMarCode

Can we update the commits to be more consistent. This will help when we need to search for specific commits in the future. E.g. something like style: fix @typescript-eslint/RULE_NAME errors

Thanks Matheus! 🙂

MrMarCode · 2025-01-14T21:17:24Z

src/utils/is-promise-like.ts

+   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   return isPromise(o) || (isObject(o) && typeof (o as any).then === 'function');


I can see how it could be confusing for a future developer, but I think it should be clear enough for at least 2 reasons:

The function's comment explains that it "returns true if o is Promise-like (i.e. has a then function)".

Developers should probably assume that the types inside a type guard are not valid until the function returns true, since that's the purpose of a type guard. If they start with that assumption, it should avoid the confusion.

MrMarCode · 2025-01-15T20:37:39Z

tests/utils/is-not-null-or-undefined.test.ts

      const arrA = [ 0, 1, 2, null, 3, undefined, 4, '', 'foo' ];

-      // Cannot assign `arrB = arrA` since arrB won't allow null or undefined.
-      let arrB: (number | string)[];
-
      const arrC = arrA.filter(t.isNotNullOrUndefined);

+      // Cannot assign `arrB = arrA` since arrB won't allow null or undefined.
      // CAN assign `arrB = arrC` since arrC has now had all null and undefined values
      // removed.
-      arrB = arrC;
+      const arrB: (number | string)[] = arrC;

      expect(arrB).to.eql([ 0, 1, 2, 3, 4, '', 'foo' ]);


It looks like arrC became somewhat redundant. Maybe we can use this opportunity to combine the assignments and simplify the comment:

it('can be used as an array filter and typeguard', () => { // Can only assign arrB here because null and undefined values are removed. const arrA = [ 0, 1, 2, null, 3, undefined, 4, '', 'foo' ], arrB: (number | string)[] = arrA.filter(t.isNotNullOrUndefined); expect(arrB).to.eql([ 0, 1, 2, 3, 4, '', 'foo' ]); });

MrMarCode · 2025-01-17T16:10:58Z

src/utils/get.ts

+function get<TResult = unknown>(
+   obj: unknown,
+   path: StringRepresentable | StringRepresentable[],
+   defaultValue?: TResult
+): TResult | undefined


This overload addresses the error in make-template.ts when getValue calls get. However, get is an exported function that could potentially be used in many places outside toolbox, whereas getValue is an internal function, only used by toolbox code. So, we can reduce the blast radius of this change by adjusting getValue instead.

This is one of the rare cases where ignoring our any rule might be warranted:

// eslint-disable-next-line @typescript-eslint/no-explicit-any function getValue(path: string, data: any): unknown {

MrMarCode · 2025-01-17T16:28:04Z

src/utils/get.ts

@@ -32,7 +32,7 @@ function toKey(value: unknown): string | symbol {
 const IS_DEEP_PROP_REGEX = /\.|\[(?:[^[\]]*|(["'])(?:(?!\1)[^\\]|\\.)*?\1)\]/,
      IS_PLAIN_PROP_REGEX = /^\w*$/;

-function isKey(value: any, object: any): boolean {
+function isKey(value: unknown, object: unknown): boolean {


After a discussion and much deliberation, we found that this file warrants ignoring our no-explicit-any rule for the following reasons:

'get' is a generic function that is meant to traverse an object, given a 'path' (string of properties, i.e get({ a: { b: { c: 'hi'}}}, 'a.b.c')). Because of the nature of the function, each key could literally reference anything. Trying to type generic code like this is near impossible and not worth it. (side note: this also implies that generic code like this is not ideal, which is true. But the use case for this is very slim. We only use it when we're receiving a string that's untrusted and we need to parse it.)

Most of the time any is used by internal functions (not exported) so the benefit of typing is not felt outside of toolbox.

A lot of this code is copied from the lodash library, which is popular, so similar code has been exercised a lot, contributing to it's reliability.

So, for all the places in this file that we switched to using unknown and ...as string can we revert it back to how it was and just ignore the rule whenever any is used:

Suggested change

function isKey(value: unknown, object: unknown): boolean {

// eslint-disable-next-line @typescript-eslint/no-explicit-any

function isKey(value: any, object: any): boolean {

// eslint-disable-next-line @typescript-eslint/no-explicit-any function createPathArray(value: any, object: any): string[] {

// eslint-disable-next-line @typescript-eslint/no-explicit-any resultObj = (resultObj as any)[toKey(pathArray[index])];

- disable rule for lines where empty function is intended

MrMarCode · 2025-01-22T15:18:41Z

src/utils/is-empty.ts

+   if (isObject(o)) {
+      return Object.keys(o).length === 0;
+   }
+   return true;


Returning true for all other types changes the behavior slightly. I know it's basically what it was doing before because it assumes that the parameter is an object at the end. But for clarity, and to have less impact on the code this might be a case where we override the type and include an explanation instead:

Suggested change

if (isObject(o)) {

return Object.keys(o).length === 0;

}

return true;

// Non-object arguments passed into Object.keys are coerced into objects (the only

// exception being undefined or null, which is handled above). Therefore, it's ok to

// assume the input is an object because it will be once passed through. See also:

// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys#using_object.keys_on_primitives

return Object.keys(o as object).length === 0;

MrMarCode

Nicely done! Thanks for working through all that 🙂

onebytegone · 2025-01-23T16:14:56Z

@MatheusBaldi Able to look at the build failure?

- isUndefined has been removed from node on version 23.x

MatheusBaldi · 2025-01-23T20:21:40Z

@onebytegone done

MatheusBaldi added 2 commits December 31, 2024 10:44

chore: update @silvermine/eslint-config digest to 634db8f

db64e4f

feat: replace eslintrc.json by eslint.config.js to use partials

73345d4

MatheusBaldi changed the title ~~chore: update eslint-config~~ chore: update @silvermine/eslint-config Jan 7, 2025

MrMarCode reviewed Jan 10, 2025

View reviewed changes

MatheusBaldi force-pushed the MatheusBaldi/update-eslint-config-file branch 2 times, most recently from 3bd569a to 536535c Compare January 14, 2025 16:07

MatheusBaldi requested a review from MrMarCode January 14, 2025 16:11

MatheusBaldi marked this pull request as ready for review January 14, 2025 16:11

MrMarCode suggested changes Jan 17, 2025

View reviewed changes

MatheusBaldi added 4 commits January 21, 2025 11:04

style: fix prefer-const eslint errors

5185b4a

style: fix @typescript-eslint/ban-types errors

2c90fb4

style: fix prefer-rest-params eslint errors

286edee

style: fix no-empty-function eslint errors

f23cd55

- disable rule for lines where empty function is intended

MatheusBaldi force-pushed the MatheusBaldi/update-eslint-config-file branch from 536535c to bdc590a Compare January 21, 2025 14:37

MrMarCode reviewed Jan 22, 2025

View reviewed changes

style: fix no-explicit-any eslint errors

5c9a19f

MatheusBaldi force-pushed the MatheusBaldi/update-eslint-config-file branch from bdc590a to 5c9a19f Compare January 23, 2025 13:04

MatheusBaldi requested a review from MrMarCode January 23, 2025 13:07

MrMarCode approved these changes Jan 23, 2025

View reviewed changes

fix: replace isUndefined from node utils by our own implementation

abcd9e0

- isUndefined has been removed from node on version 23.x

MatheusBaldi force-pushed the MatheusBaldi/update-eslint-config-file branch from 8600a42 to abcd9e0 Compare January 23, 2025 20:16

onebytegone merged commit 03a368b into silvermine:master Jan 23, 2025
6 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update @silvermine/eslint-config #105

chore: update @silvermine/eslint-config #105

MatheusBaldi commented Jan 7, 2025 •

edited

Loading

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MatheusBaldi Jan 14, 2025 •

edited

Loading

MrMarCode Jan 14, 2025

MrMarCode Jan 10, 2025

MrMarCode Jan 10, 2025

MatheusBaldi Jan 14, 2025

MrMarCode left a comment

MrMarCode Jan 14, 2025

MrMarCode Jan 15, 2025

MrMarCode Jan 17, 2025

MrMarCode Jan 17, 2025

MrMarCode Jan 22, 2025

MrMarCode left a comment

onebytegone commented Jan 23, 2025

MatheusBaldi commented Jan 23, 2025

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		return isPromise(o) \|\| (isObject(o) && typeof (o as any).then === 'function');

		// eslint-disable-next-line no-proto, @typescript-eslint/no-explicit-any
		(objWithProto as any).__proto__ = { b: 2 };

	function isKey(value: unknown, object: unknown): boolean {
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	function isKey(value: any, object: any): boolean {

-   if (isObject(o)) {
-      return Object.keys(o).length === 0;
-   }
-   return true;
+   // Non-object arguments passed into Object.keys are coerced into objects (the only
+   // exception being undefined or null, which is handled above). Therefore, it's ok to
+   // assume the input is an object because it will be once passed through. See also:
+   // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys#using_object.keys_on_primitives
+   return Object.keys(o as object).length === 0;

chore: update @silvermine/eslint-config #105

chore: update @silvermine/eslint-config #105

Conversation

MatheusBaldi commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatheusBaldi Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrMarCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrMarCode left a comment

Choose a reason for hiding this comment

onebytegone commented Jan 23, 2025

MatheusBaldi commented Jan 23, 2025

MatheusBaldi commented Jan 7, 2025 •

edited

Loading

MatheusBaldi Jan 14, 2025 •

edited

Loading