ref(utils): Simplify value normalization - #4666
Conversation
size-limit report
|
| } | ||
| } | ||
| return [memoize, unmemoize]; | ||
| return { memoize, unmemoize }; |
There was a problem hiding this comment.
The tuple return was done for bundle size reasons btw - I think it's reasonable enough to leave as is as long as we comment it properly.
There was a problem hiding this comment.
I know, but IMHO we've gone too far with bundle size when we let it make our code confusing to read, which it was when I first was looking into this.
There was a problem hiding this comment.
Thats a fair point. Can we perhaps use array destructuring to avoid the memo[0] calls?
const [memoize, unmemoize] = memo;There was a problem hiding this comment.
Sure, here I think that's fine. I'll make the change. In general, though, I'm of the mind that bundle size is important but that I also don't want to get to a point where we feel like we can't use objects when they make sense.
Help me remember, though, please. As I understand it, the reason a destructured array is better than an object (or even a destructured object) is that terser won't mangle the property names because they count as public API, whereas with an array the local names used in the destructuring will get mangled? Would it still count as public API if memoBuilder were to be defined in the same file where it's used (and were not to be exported, since it's not used anywhere else)? No, right?
(I'm asking partly just so I understand what we're talking about. I recognize that making that change would also remove memoBuilder from @sentry/utils's public API, and so would have to wait for v7 if it were to happen at all. But if I'm right, it might be worth looking at what else we export from utils which we don't actually need to.)
There was a problem hiding this comment.
Regardless of if memoBuilder is exported or not, { memoize, unmemoize } means that memoize and unmemoize are unminifiable. The array destructuring just helps with that.
|
Yep, I think this is in the right direction. I still need to try and reproduce some of the things that currently trip it up. My best guess is that it's something to do with DOM elements. Another thing that randomly popped into my head while laying bed the other night was... arrays. Do we do anything to limit their length? |
ceaf57e to
07af2a4
Compare
|
I opened #4676 - I think we should explore fuzzing the serialization logic. |
07af2a4 to
e2c27d6
Compare
3 participants
This PR makes a few changes to the internals of
utils.normalize:Instead of using
JSON.stringifyto handle the recursive visiting of nodes in an object tree (and then giving it a visit function (walk) which is itself a recursive visitor of nodes), forcing us then to callJSON.parseon the results, it simply uses said recursive visitor to recursively visit the object tree nodes.It renames
normalizeValuetomakeSerializable(since that's more accurately and specifically what it's doing) and updates its out-of-date docstring to match its behavior.It refers to the methods in our memoizer by name rather than number, splits out an inlined value, and moves a typecast, all to increase readability.
Note: This was a branch I had lying around, about which I was reminded while reading #4579. I don't think this will help with that a ton, but I think it can't hurt.