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.
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
feat: do not serialize functions or constructors that are not invoked. #8
Conversation
thantos
commented
Jun 22, 2022
•
edited
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job.
src/closure/createClosure.ts
Outdated
@@ -635,10 +642,11 @@ async function analyzeFunctionInfoAsync( | |||
logInfo | |||
); | |||
|
|||
// try to only serialize out the properties that were used by the user's code. | |||
// try to only serialize out the pro perties that were used by the user's code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo
src/closure/createClosure.ts
Outdated
capturedObjectProperties && | ||
capturedObjectProperties.length > 0 | ||
) { | ||
entry.object = entry.object || { env: new Map() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to use ??
instead of ||
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why new Map()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copies this from elsewhere in the code, lol, but it appears that object needs to be a map or else things start to fail.
src/closure/createClosure.ts
Outdated
serialize, | ||
logInfo | ||
); | ||
// if not invoked, let the property be treated like an object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meaning attempt to only serialize used properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. tree-shake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, previous behavior was that anything recognized as a function instanceof Function
would be fully serialized, meaning static properties made a mess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And namespaces more so :-)