style(eslint): apply specified code style changes #138

DerekNonGeneric · 2022-10-16T01:21:05Z

This PR separates the code style changes proposed by @ignoreintuition in #133.

Although recognized as desirable, they would be beyond the scope of the specific feature we are working on in that workstream; breaking them out here so as not to derail would be preferable.

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric · 2022-10-16T01:32:05Z

Alright, so we already have some good feedback to work with on this PR. It seems to have made many corrections but would have been introducing an antipattern that we do not currently lint for with our local tooling:

Use of a banned type detected ^JS-0296

Don't use Object as a type

— https://deepsource.io/gh/openinf/util-object/run/0db33e3f-7492-4a1c-bbf1-68a2271cdf54/javascript/JS-0296?listindex=0

DerekNonGeneric · 2022-10-16T02:15:53Z

src/index.ts

@@ -84,10 +83,10 @@ interface ITargetSourceDepth {
 */
 export function deepMerge(target: Object, source: Object, depth = 10): Object {
  // Keep track of seen objects to detect recursive references.
-  const seen: Array<Object> = [];
+  const seen: Object[] = [];


I have started to collect some research on the topic at https://github.com/openinf/util-object/wiki/Type-Annotation-for-JavaScript-Objects, which we'll be incorporating as part of our style guide. Once I have settled on our preference, i'll be sure to make the update here.

/cc @septs as this would be another guideline to be added to our JS authoring guide (and enforced via our standard) ^^

Object -> object
Number -> number
String -> string
BigInt -> bigint
Symbol -> symbol
Boolean -> boolean

Object -> object

For this one, that would seem alright, but it would be very loose as it would simply mean any non-nullish value. Wonder what you think about the TypeScript Record utility type?

I don't like that it is called the same as Record from the TC39 proposal, seeing as how they have different semantics iiuc. The TypeScript Record utility type isn't an immutable object afaik, so mixing that in our codebase is a bit annoying imo.

septs · 2022-10-16T04:03:01Z

src/index.ts

  }
-  return obj;
+  return object;


Suggested change

return object;

return { ...opt_initial };

That might work; need to double-check, but just so that we're on the same page, this is what we are referred to when we say that it returns a "map-like object":

https://github.com/google/closure-compiler/wiki/A-word-about-the-type-Object#map-like-objects

src/index.ts

septs · 2022-10-16T04:04:21Z

src/index.ts

-  } else {
-    return undefined;
-  }
+export function ownProperty(


see https://mdn.io/relect-get

see https://mdn.io/relect-get

I believe the Reflect.get static method would also be able to access any inherited properties (e.g., from the base Object superclass — all the properties from the constructor would be included), which i think we'd like to avoid.

For security purposes, i believe only having access to all enumerable own properties would be safer and more efficient iiuc.

septs · 2022-10-16T04:05:18Z

src/index.ts

@@ -160,14 +159,14 @@ export function objectsEqualShallow(
 * @template T,R
 */
 export function memo<T, P extends keyof T>(


see https://github.com/tc39/proposal-function-memo

Yeah, i knew about that proposal, but am not yet sure how it relates to this function. Is the difference that this is Object memo and that would be Function memo?

/cc @js-choi

spread props don't trigger getters on the target object Refs: https://babeljs.io/docs/en/assumptions#setspreadproperties Co-authored-by: septs <[email protected]> Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

src/index.ts

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric · 2022-10-17T00:11:03Z

Cool, we finally got a green CI 🎉

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric · 2022-10-17T00:18:21Z

Oh dang, the TypeScript Compiler just type-checked the file and found some nits 😅

Refs: https://github.com/openinf/util-object/actions/runs/3261462228/jobs/5356396292

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric · 2022-10-17T02:27:04Z

Having trouble interpreting the TS typecheck error in CI rn, but am going to land it as-is since it's blocking a lot of other project-critical work and will likely take longer to resolve than would be worthwhile at this point.

We'll be circling back to this shortly and obviously will have to resolve it before release.

DerekNonGeneric · 2022-10-18T01:12:18Z

I have made a comment pertaining to a change that we have made here.

See: 34fdb2a#r87092905

Refs: https://babeljs.io/docs/en/assumptions#setspreadproperties Fixes: #141 Fixes: #139 PR-URL: #138 Reviewed-by: septs <[email protected]> Reviewed-by: Derek Lewis <[email protected]> Signed-off-by: Derek Lewis <[email protected]> Co-authored-by: Derek Lewis <[email protected]> Co-authored-by: septs <[email protected]>

style(eslint): apply specified code style changes

54574c8

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

DerekNonGeneric mentioned this pull request Oct 16, 2022

(JS-0296) Use of a banned type detected #139

Closed

septs reviewed Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

septs reviewed Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

septs reviewed Oct 16, 2022

View reviewed changes

fix(src): use object spread

ebe562d

spread props don't trigger getters on the target object Refs: https://babeljs.io/docs/en/assumptions#setspreadproperties Co-authored-by: septs <[email protected]> Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

fix(src): use object literal instead of banned type

c33049e

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

style(index): prefer tailing commas for js objects

c34389b

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

fix(src): update to use trailing commans for js objects

607e6c0

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

fix

24ced7d

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

3209b2a

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

ffc58ba

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

DerekNonGeneric commented Oct 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

DerekNonGeneric added 4 commits October 16, 2022 14:05

Update src/index.ts

6da9d15

Signed-off-by: Derek Lewis <[email protected]>

Update src/index.ts

96e08c3

Signed-off-by: Derek Lewis <[email protected]>

Update src/index.ts

8e963d1

Signed-off-by: Derek Lewis <[email protected]>

Update src/index.ts

fcb48a3

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric added 3 commits October 16, 2022 20:01

Update src/index.ts

74f22dc

Signed-off-by: Derek Lewis <[email protected]>

Update src/index.ts

d13b925

Signed-off-by: Derek Lewis <[email protected]>

Update src/index.ts

4545791

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 17, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

1b80afe

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 17, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

a14acd2

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 17, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

881139f

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 17, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

c2f6013

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 17, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

c077e4f

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric commented Oct 17, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Update src/index.ts

934c84e

Signed-off-by: Derek Lewis <[email protected]>

Merge branch 'main' into style/ignoreintuition

b137820

Signed-off-by: Derek Lewis <[email protected]>

fix(src): address remaining typecheck issues

4fc93ee

Signed-off-by: Derek Lewis <[email protected]>

DerekNonGeneric merged commit 34fdb2a into OpenINF:main Oct 17, 2022

DerekNonGeneric deleted the style/ignoreintuition branch October 17, 2022 02:31

DerekNonGeneric added area: typescript area: assumptions typings area: eslint area: comments labels Oct 18, 2022

DerekNonGeneric mentioned this pull request Oct 18, 2022

fix(src): map to return object .prototype undefined #147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(eslint): apply specified code style changes #138

style(eslint): apply specified code style changes #138

DerekNonGeneric commented Oct 16, 2022

DerekNonGeneric commented Oct 16, 2022

DerekNonGeneric Oct 16, 2022

septs Oct 16, 2022

DerekNonGeneric Oct 16, 2022

DerekNonGeneric Oct 16, 2022

septs Oct 16, 2022

DerekNonGeneric Oct 16, 2022

septs Oct 16, 2022

DerekNonGeneric Oct 16, 2022 •

edited

Loading

septs Oct 16, 2022

DerekNonGeneric Oct 16, 2022

DerekNonGeneric commented Oct 17, 2022

DerekNonGeneric commented Oct 17, 2022 •

edited

Loading

DerekNonGeneric commented Oct 17, 2022

DerekNonGeneric commented Oct 18, 2022 •

edited

Loading

style(eslint): apply specified code style changes #138

style(eslint): apply specified code style changes #138

Conversation

DerekNonGeneric commented Oct 16, 2022

DerekNonGeneric commented Oct 16, 2022

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

DerekNonGeneric Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerekNonGeneric commented Oct 17, 2022

DerekNonGeneric commented Oct 17, 2022 • edited Loading

DerekNonGeneric commented Oct 17, 2022

DerekNonGeneric commented Oct 18, 2022 • edited Loading

DerekNonGeneric Oct 16, 2022 •

edited

Loading

DerekNonGeneric commented Oct 17, 2022 •

edited

Loading

DerekNonGeneric commented Oct 18, 2022 •

edited

Loading