Replace lodash with native js #753

Shinigami92 · 2021-03-04T19:26:12Z

fixes #738

src/operations/tables.ts

Shinigami92 · 2021-03-04T20:13:05Z

src/operations/tables.ts

-      (options: ColumnDefinition, columnName) =>
-        typeof options.comment !== 'undefined' &&
-        makeComment('COLUMN', `${mOptions.literal(tableName)}.${mOptions.literal(columnName)}`, options.comment),
+  const comments = Object.keys(columnsWithOptions).filter((columnName) => {


This has no unit test coverage

Shinigami92 · 2021-03-04T20:19:11Z

Mh 🤔, seems that 360cb7c broke it 🙁
I need to look deeper into this chain function

Shinigami92 · 2021-03-04T20:53:26Z

@goce-cz Okay I'm really confused why this is not working 🙁
Could you have a look into the e2e tests and hopefully provide me with a hint what the problem could be?

This reverts commit 360cb7c.

Shinigami92 · 2021-03-05T07:25:53Z

Okay I check it by reverting my last 3 commits and yes, it is definitely the _.chain call that I currently misunderstand 🤔
I will ask for help on a TypeScript Community Discord Server 🙂

Shinigami92 · 2021-03-05T07:47:20Z

Aha! The primaryColumns are correct but the comments are not

columnsWithOptions before comments: { id:
   { type: 'serial',
     primaryKey: true,
     comment: 'comment on column id' } }
comments with chain: [ 'COMMENT ON COLUMN "test"."tcc"."id" IS $pga$comment on column id$pga$;' ]
comments with native: [ 'id' ]

Shinigami92 · 2021-03-05T07:57:05Z

We may extract the following to its own interface

{
  [x: string]: ColumnDefinition & FunctionParamType
}

Or we could replace it with Record<string, ColumnDefinition & FunctionParamType>

Or both solutions together:

export type NewType = Record<string, ColumnDefinition & FunctionParamType>
// ...
Object.keys(/*...*/).reduce<NewType>(// ...

Shinigami92 · 2021-03-05T07:57:39Z

@goce-cz If merging this PR, you should prefer a squash-merge 🙂

goce-cz

Thank you very much for your contribution. I agree that using lodash for primitive operations like Array.map() and Object.keys() is a relic from the times long gone and having it in a library like this is a signal of malfunctioning maintenance 👍

The non-trivial helpers like intersect are a different question, but it probably makes little sense to depend on whole lodash because a few usages of simple helpers.

Please polish the types and variable names as suggested and also consider using Object.entries instead of Object.keys where appropriate.

goce-cz · 2021-03-18T12:27:51Z

src/operations/tables.ts

+  let columnsWithOptions = Object.keys(columns).reduce<{
+    [x: string]: ColumnDefinition & FunctionParamType
+  }>((previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }), {})


Ideally this would be:

Suggested change

let columnsWithOptions = Object.keys(columns).reduce<{

[x: string]: ColumnDefinition & FunctionParamType

}>((previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }), {})

let columnsWithOptions = Object.fromEntries(

Object.entries(columns)

.map(([columnName, columnDefinition]) =>

[columnName, applyType(columnDefinition,extendingTypeShorthands)]

)

)

too bad that Object.fromEntries is not supported prior Node 14...

I'm not a great fan of allocating a new object in each iteration of the reduce operation, but I guess it's a common practice and we probably don't expect thousands of columns here. Anyway I'd still prefer if you used Object.entries() to avoid dependency on the source object from within the callback:

Suggested change

let columnsWithOptions = Object.keys(columns).reduce<{

[x: string]: ColumnDefinition & FunctionParamType

}>((previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }), {})

let columnsWithOptions = Object.entries(columns).reduce<{

[x: string]: ColumnDefinition & FunctionParamType

}>((previous, [columnName, columnDefinition]) => ({ ...previous, [columnName]: applyType(columnDefinition, extendingTypeShorthands) }), {})

Regarding the type, I would just use Record<> instead to make the code more readable and keep the explicit type extension.

Suggested change

let columnsWithOptions = Object.keys(columns).reduce<{

[x: string]: ColumnDefinition & FunctionParamType

}>((previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }), {})

let columnsWithOptions: Record<string, ColumnDefinition & FunctionParamType> = Object.keys(columns).reduce(

(previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }),

{},

)

I think I can use Object.fromEntries because the build step targets es2017

https://node.green/#ES2019-features--Object-fromEntries

But looking into the produced dist folder, the Object.fromEntries is still there 🤔

Should we drop node 10 support?
node 12 supports it

Using the reduce or eventually some custom helper is IMO not a big enough annoyance to justify dropping support of Node 10. 🤷

We can deprecate the old Node with the next major/minor release (issue a warning) and then drop it later.

TL;DR; Please remove the Object.fromEntries for now - the tests are failing.

src/operations/tables.ts

src/operations/types.ts

Shinigami92 · 2021-03-18T13:14:52Z

Thanks for the really great review 🚀
I will look into this this evening and apply changes

I also thought about extracting an intersection-function to a utility file

goce-cz

Please see below:

goce-cz · 2021-04-23T08:05:38Z

src/operations/tables.ts

+  let columnsWithOptions = Object.keys(columns).reduce<{
+    [x: string]: ColumnDefinition & FunctionParamType
+  }>((previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }), {})


Using the reduce or eventually some custom helper is IMO not a big enough annoyance to justify dropping support of Node 10. 🤷

We can deprecate the old Node with the next major/minor release (issue a warning) and then drop it later.

TL;DR; Please remove the Object.fromEntries for now - the tests are failing.

Shinigami92 · 2021-04-24T09:13:22Z

@goce-cz All tests are green ✔️

goce-cz

Thanks a lot 👍

Shinigami92 added 5 commits March 4, 2021 20:22

Replace lodash isArray with native Array.isArray

2be94a7

Replace simple map calls

a219c6a

Replace lodash mapValue calls

25b5de5

Replace lodash keys with Object.keys

21db9ed

Replace lodash intersection with native filter

fa7ff2a

Shinigami92 commented Mar 4, 2021

View reviewed changes

src/operations/tables.ts Show resolved Hide resolved

Shinigami92 added 4 commits March 4, 2021 21:00

Replace lodash map obj with native Object.keys and map

6c6045d

Remove lodash import

d7997ad

Replace lodash chain with native Object.keys and filter

360cb7c

Remove lodash dependencies

5cbfd82

Shinigami92 commented Mar 4, 2021

View reviewed changes

Add temporary console.log

7b34fd2

Revert "Replace lodash chain with native Object.keys and filter"

29c9237

This reverts commit 360cb7c.

Add temporary console.log

6ca7d99

Shinigami92 added 2 commits March 5, 2021 08:48

Replace lodash chain with native Object.keys, filter and map

41e1546

Remove lodash dependencies

96a80d9

Shinigami92 marked this pull request as ready for review March 5, 2021 07:54

goce-cz requested changes Mar 18, 2021

View reviewed changes

Shinigami92 self-assigned this Mar 18, 2021

Shinigami92 added 2 commits March 18, 2021 17:28

Use Object.entries

21f514a

Add intersection util function

3766bc3

Shinigami92 requested a review from goce-cz March 18, 2021 16:51

goce-cz requested changes Apr 23, 2021

View reviewed changes

Restore node v10 compatibility

20d8f67

Shinigami92 requested a review from goce-cz April 23, 2021 14:14

goce-cz approved these changes Apr 27, 2021

View reviewed changes

goce-cz merged commit 4e3e892 into salsita:master Apr 27, 2021

Shinigami92 deleted the issue-738 branch April 27, 2021 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace lodash with native js #753

Replace lodash with native js #753

Shinigami92 commented Mar 4, 2021

Shinigami92 Mar 4, 2021 •

edited

Loading

Shinigami92 commented Mar 4, 2021

Shinigami92 commented Mar 4, 2021

Shinigami92 commented Mar 5, 2021

Shinigami92 commented Mar 5, 2021

Shinigami92 commented Mar 5, 2021

Shinigami92 commented Mar 5, 2021

goce-cz left a comment

goce-cz Mar 18, 2021

goce-cz Mar 18, 2021

Shinigami92 Mar 18, 2021 •

edited

Loading

Shinigami92 Mar 18, 2021

goce-cz Apr 23, 2021

Shinigami92 commented Mar 18, 2021

goce-cz left a comment

goce-cz Apr 23, 2021

Shinigami92 commented Apr 24, 2021

goce-cz left a comment

-  let columnsWithOptions = Object.keys(columns).reduce<{
-    [x: string]: ColumnDefinition & FunctionParamType
-  }>((previous, column) => ({ ...previous, [column]: applyType(columns[column], extendingTypeShorthands) }), {})
+  let columnsWithOptions = Object.fromEntries(
+    Object.entries(columns)
+      .map(([columnName, columnDefinition]) =>
+        [columnName, applyType(columnDefinition,extendingTypeShorthands)]
+      )
+  )

Replace lodash with native js #753

Replace lodash with native js #753

Conversation

Shinigami92 commented Mar 4, 2021

Shinigami92 Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Shinigami92 commented Mar 4, 2021

Shinigami92 commented Mar 4, 2021

Shinigami92 commented Mar 5, 2021

Shinigami92 commented Mar 5, 2021

Shinigami92 commented Mar 5, 2021

Shinigami92 commented Mar 5, 2021

goce-cz left a comment

Choose a reason for hiding this comment

goce-cz Mar 18, 2021

Choose a reason for hiding this comment

goce-cz Mar 18, 2021

Choose a reason for hiding this comment

Shinigami92 Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Shinigami92 Mar 18, 2021

Choose a reason for hiding this comment

goce-cz Apr 23, 2021

Choose a reason for hiding this comment

Shinigami92 commented Mar 18, 2021

goce-cz left a comment

Choose a reason for hiding this comment

goce-cz Apr 23, 2021

Choose a reason for hiding this comment

Shinigami92 commented Apr 24, 2021

goce-cz left a comment

Choose a reason for hiding this comment

Shinigami92 Mar 4, 2021 •

edited

Loading

Shinigami92 Mar 18, 2021 •

edited

Loading