-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
optimize __createBinding
#46997
optimize __createBinding
#46997
Conversation
0dc4b00
to
7733452
Compare
I did the change and added also a bit that does the same optimization of using an existing descriptor if the binding is marked as non-writable and non-configurable -- which means that it will be used as-is for the value too. This makes it possible to hack around the need for a getter if you know that your code does not modify exported bindings (it could be done by tsc in the future). For example:
(BTW, I removed the redundant parens, but now that the condition is much longer, I added parens around the whole thing which I think makes it more readable, but will drop them if you prefer no redundant parentheses at all...) |
7733452
to
2a983d8
Compare
src/compiler/factory/emitHelpers.ts
Outdated
var desc = (oldDesc.writable === false && oldDesc.configurable === false | ||
|| m.__esModule && "get" in oldDesc) | ||
? oldDesc : { enumerable: true, get: function() { return m[k]; } }; |
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'm curious about the conditions here:
oldDesc.configurable === false
could just be!oldDesc.configurable
, becauseconfigurable
is always set by the runtime.oldDesc.writable === false
means there won't be a"get"
in oldDesc, becausewritable
is only added for data properties (and isundefined
for accessor properties).- There's a possibility that
oldDesc
is undefined (if re-exporting something from a dynamic module), in which caseoldDesc.writable
could throw
Would the following condition also match the intended behavior here?
var desc = (oldDesc.writable === false && oldDesc.configurable === false | |
|| m.__esModule && "get" in oldDesc) | |
? oldDesc : { enumerable: true, get: function() { return m[k]; } }; | |
var desc = oldDesc && ("get" in oldDesc ? m.__esModule : !oldDesc.writable && !oldDesc.configurable) | |
? oldDesc : { enumerable: true, get: function() { return m[k]; } }; |
By branching on "get" in oldDesc"
early, we can just use !
for the writable
/configurable
tests, since oldDesc.writable
will never be false
(or true
) for an accessor.
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.
Alternatively, this could be rewritten into an if
and a reassignment (which is a bit more readable):
var desc = Object.getOwnPropertyDescriptor(m, k);
if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) {
desc = { enumerable: true, get: function() { return m[k]; } };
}
Object.defineProperty(o, k2, desc);
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.
All of that makes sense, will push that in a minute.
- Re using
!
, I guess that it makes sense since it's part of the descriptor spec (I was focused on actual node behavior wrt cjs bindings from a frozenexports
, which I wanted to make as robust as possible). writable
vsget
-- makes sense.- And the reassignment is a nice improvement though the resulting negation of the condition makes it more confusing, so I don't care either way.
When the binding is itself one that was created by `__createBinding`, re-use its descriptor, which avoids piling multiple levels of getters in the case of multiple levels of exports. In addition, reuse a descriptor if the bindings is marked as non-writable and non-configurable, which makes a getter not necessary. (This can be done manually if needed, even though tsc doesn't do it now.) Also related to microsoft#46744 and to microsoft/tslib#165.
2a983d8
to
0876b6b
Compare
Is there a PR for tslib yet? |
Reflect microsoft/TypeScript#46997: When the binding is itself one that was created by `__createBinding`, re-use its descriptor, which avoids piling multiple levels of getters in the case of multiple levels of exports. In addition, reuse a descriptor if the bindings is marked as non-writable and non-configurable, which makes a getter not necessary. (This can be done manually if needed, even though tsc doesn't do it now.) Could be considered as a fix for #165 -- first, this PR prevents piling up multiple layers of getters. Second, it allows a hack of adding if (typeof exports === "object") exports = Object.freeze(exports); to avoid getters altogether. (And in the future, tsc could mark `const` exports as non-writable and non-configurable which would make it possible to avoid this hack.)
Made one now: microsoft/tslib#168 |
Reflect microsoft/TypeScript#46997: When the binding is itself one that was created by `__createBinding`, re-use its descriptor, which avoids piling multiple levels of getters in the case of multiple levels of exports. In addition, reuse a descriptor if the bindings is marked as non-writable and non-configurable, which makes a getter not necessary. (This can be done manually if needed, even though tsc doesn't do it now.) Could be considered as a fix for #165 -- first, this PR prevents piling up multiple layers of getters. Second, it allows a hack of adding if (typeof exports === "object") exports = Object.freeze(exports); to avoid getters altogether. (And in the future, tsc could mark `const` exports as non-writable and non-configurable which would make it possible to avoid this hack.)
Reflect microsoft/TypeScript#46997: When the binding is itself one that was created by `__createBinding`, re-use its descriptor, which avoids piling multiple levels of getters in the case of multiple levels of exports. In addition, reuse a descriptor if the bindings is marked as non-writable and non-configurable, which makes a getter not necessary. (This can be done manually if needed, even though tsc doesn't do it now.) Could be considered as a fix for #165 -- first, this PR prevents piling up multiple layers of getters. Second, it allows a hack of adding if (typeof exports === "object") exports = Object.freeze(exports); to avoid getters altogether. (And in the future, tsc could mark `const` exports as non-writable and non-configurable which would make it possible to avoid this hack.)
When the binding is itself one that was created by
__createBinding
,re-use its descriptor, which avoids piling multiple levels of getters in
the case of multiple levels of exports.
Also related to #46744 and to microsoft/tslib#165.