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.

Sign up for GitHub

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

Jump to bottom

fix: replace merge() with Object.assign() #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: replace merge() with Object.assign() #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,13 @@
"execa": "^5.1.1",
"floggy": "^0.2.3",
"fs-jetpack": "^4.1.1",
"lodash": "^4.17.21",
"playwright": "^1.15.1",
"tslib": "^2.3.1"
},
"devDependencies": {
"@homer0/prettier-plugin-jsdoc": "4.0.5",
"@prisma-labs/prettier-config": "0.1.0",
"@types/jest": "27.0.1",
"@types/lodash": "^4.14.173",
"@types/node": "16.9.1",
"@types/ts-nameof": "4.2.1",
"@typescript-eslint/eslint-plugin": "4.31.0",
Expand Down
3 changes: 1 addition & 2 deletions src/runners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { merge } from 'lodash'
import ono from '@jsdevtools/ono'
import { HookNames, jestHookLookup, jestHookToPhase } from './jest'
import { kontLog } from './log'
Expand Down Expand Up @@ -35,7 +34,7 @@ export const runSetup = (params: {
)
}

merge(params.currentContext, contributedContext ?? {})
Object.assign(params.currentContext, contributedContext ?? {})
Copy link
Member

Choose a reason for hiding this comment

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

This will do a shallow merge, we should actually add tests and docs about how merging works.

We definitely want a deep merge.

But maybe we do have something not desirable about the lodash algo. Can you investigate an isolated example of how the merge is not doing what you want/expect?

Copy link
Author

Choose a reason for hiding this comment

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

What is the benefit of a deep merge? Will reference what I am doing as soon as the PR working in general. Will probably send it over tomorrow.

I want some of my values in my context mutable from within itself. But maybe there's an error with my code.

Copy link
Member

@jasonkuhrt jasonkuhrt Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is pretty case dependent. Happy to look at your case though. Maybe we need a toggle to enable or disable deep merge e.g.:

import { konn } from 'konn'

konn.settings({ deepMerge: false })

Note that this has an impact on the static types. The static types currently do a deep merge. The only way to to get the static typing change from this runtime setting change would be either some pretty complex typegen stuff or, more simply, via keeping the settings change in the chain. Reuse can be achieved like so:

import { konn } from 'konn'

export const mykonn = konn.settings({...})

Now all code needs to import and use mykonn instead of konn.

Providers API doesn't need to worry about this I think.

Making static types not do a deep merge will take some additional effort in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

I would expect it to simply blow up if the keys collide as there's no good way of approaching it - both merging and overriding have drawbacks - I'd treat it as an implementation error.

In tRPC I have a bunch of similar cases and the way I deal with it is that I make the server explode on bootup if you accidentally achieve "impossible" states (duplicate queries/multiple data transformers/[..]).

Copy link
Member

Choose a reason for hiding this comment

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

@KATT Strict mode sounds nice, I see a union instead of boolean then:

konn.settings({ merge: 'error' | 'deep' | 'shallow' })

Copy link
Author

Choose a reason for hiding this comment

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

If you don't actually utilize the behavior of being able to deep merge props in any of your apps already, I think you should just delete it. Always great to opt for keeping the API surface area slim & keeping deps to a minimum when making libs.


log.trace('did_setup', { contributedContext })
})
Expand Down
9 changes: 0 additions & 9 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,13 +1185,6 @@ __metadata:
languageName: node
linkType: hard

"@types/lodash@npm:^4.14.173":
version: 4.14.173
resolution: "@types/lodash@npm:4.14.173"
checksum: 9e97ef5816299e5470db1cb32a93e981af60f74f18a35d045ed4caf224a065df96bfae6e444ec96aa392fc01258592b965d840ae042eef77ef719a578c7daef8
languageName: node
linkType: hard

"@types/ms@npm:*":
version: 0.7.31
resolution: "@types/ms@npm:0.7.31"
Expand Down Expand Up @@ -4520,7 +4513,6 @@ fsevents@^2.3.2:
"@jsdevtools/ono": ^7.1.3
"@prisma-labs/prettier-config": 0.1.0
"@types/jest": 27.0.1
"@types/lodash": ^4.14.173
"@types/node": 16.9.1
"@types/ts-nameof": 4.2.1
"@typescript-eslint/eslint-plugin": 4.31.0
Expand All @@ -4537,7 +4529,6 @@ fsevents@^2.3.2:
jest-watch-select-projects: 2.0.0
jest-watch-suspend: 1.1.2
jest-watch-typeahead: 0.6.4
lodash: ^4.17.21
markdown-toc: ^1.2.0
playwright: ^1.15.1
prettier: 2.4.0
Expand Down