chore(suite): use structuredClone for deep copying #6071
Conversation
dahaca
requested review from
tomasklim,
matejkriz,
mroz22,
szymonlesisz and
sime
as code owners
| export const getBitcoinNetwork = (pathOrName: number[] | string) => { | ||
| const networks = cloneCoinInfo(bitcoinNetworks); | ||
| const networks: BitcoinNetworkInfo[] = window.structuredClone(bitcoinNetworks); |
There was a problem hiding this comment.
It shouldn't be necessary to use window and we should avoid that, because it will break code in other envs like RN or Node
There was a problem hiding this comment.
yes, this is a no go in connect-code. window is only allowed in connect-web package. I thinking if we could improve tooling to avoid this next time?
There was a problem hiding this comment.
tests caught that as expected https://github.com/trezor/trezor-suite/runs/7961088382?check_suite_focus=true#step:7:227 still would be great to improve tsconfig to forbit using of window in ths project
There was a problem hiding this comment.
I apologize, of course -_- 13a81ec
There was a problem hiding this comment.
@mroz22 maybe eslint will be better for this kind of stuff
| @@ -52,7 +41,7 @@ export const getEthereumNetwork = (pathOrName: number[] | string) => { | |||
| }; | |||
|
|
|||
| export const getMiscNetwork = (pathOrName: number[] | string) => { | |||
| const networks = cloneCoinInfo(miscNetworks); | |||
| const networks: MiscNetworkInfo[] = window.structuredClone(miscNetworks); | |||
There was a problem hiding this comment.
Did you checked output in /lib that this is really correctly transpiled and polyfilled?
There was a problem hiding this comment.
even if it polyfilled correctly you would still need to persuade me that it is good :D
There was a problem hiding this comment.
Why not? It is better to use build-in api than something DIY.
There was a problem hiding this comment.
Nope, it stays as just structuredClone() :(
There was a problem hiding this comment.
I don't think it works in the connect part at all actually because of that
There was a problem hiding this comment.
from the discussion about backporting this to Node v16 (currently available from v17) nodejs/node#40756 (comment) 🤷
There was a problem hiding this comment.
maybe it's better to wait until we upgrade to a higher version of Node to start using this?
There was a problem hiding this comment.
It is also running in the browser in iframe right? https://caniuse.com/mdn-api_structuredclone It is not supported in all Suite supported browsers. Connect should be supported in even more browsers than Suite.
There was a problem hiding this comment.
I created a separate util after all and only use it if available as discussed with @Nodonisko c21c178
There was a problem hiding this comment.
Seems it is not polyfilling correctly so let's better wait
dahaca
force-pushed
the
feat/structured-clone
branch
from
13a81ec to
c21c178
Compare
| @@ -0,0 +1,15 @@ | |||
| export const deepClone = <T>(value: T): T => { | |||
There was a problem hiding this comment.
There are ~ 12 more places where JSON.parse(JSON.stringify... is used, I guess it would be nice to use this util on all those places?
| @@ -0,0 +1,15 @@ | |||
| export const deepClone = <T>(value: T): T => { | |||
There was a problem hiding this comment.
I would suggest little bit restrict values that can be passed, because you can pass anything now I think, but I am not sure how JSON will behave if you will pass object with BigInt or other special types. It will work for structuredClone but JSON could mess it up, so we should limit it only types that will work 100% for both solutions.
There was a problem hiding this comment.
And now we are back to square 1 from which the conversation has started hehe :D Yeah, makes sense.
There was a problem hiding this comment.
I agree that we should consider that objects are going to be stringified when using JSON.pasrse(JSON.stringify....
I am not worried about current places where it is used (because it is already working) but in potential new uses where we think that this behaves like structuredClone and in in some scenarios it will not.
const appointment = {
location: 'Prague',
day: new Date(),
}
const whenStructuredClone = structuredClone(appointment)
whenStructuredClone.day.getDate()
// result -> 25
const whenStringified = JSON.parse(JSON.stringify(appointment))
whenStringified.day.getDate()
// result -> Uncaught TypeError: whenStringified.day.getDate is not a functionAnd the same for nested objects. And the same behavior can be expected from other objects like BigInt.
Restricting the use to types that will work 100% for both could be a solution but if I am not mistaken I think it should iterate over the whole object and nested potential objects to make sure there is no nested BigInt or other not working type.
There was a problem hiding this comment.
I think type like this should do the job:
import { JsonValue } from 'type-fest'
| if (typeof structuredClone === 'function') { | ||
| clone = structuredClone(value); | ||
| } else { | ||
| clone = JSON.parse(JSON.stringify(value)); |
There was a problem hiding this comment.
We should probably use some better polyfill than this. There are already available.
There was a problem hiding this comment.
Never heard of it before, core-js has a polyfill. (I know we are cautious about included new dependencies, but just sharing).
|
let's wait for Node v17, closing as stale |
|
Agree |
Description
Use
window.structuredClone()for deep copying