Merged
Conversation
rbuckton
suggested changes
Dec 10, 2021
ExE-Boss
reviewed
Jan 8, 2022
0dc4b00 to
7733452
Compare
Contributor
Author
|
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
rbuckton
suggested changes
Jan 12, 2022
src/compiler/factory/emitHelpers.ts
Outdated
Comment on lines
+787
to
+789
| var desc = (oldDesc.writable === false && oldDesc.configurable === false | ||
| || m.__esModule && "get" in oldDesc) | ||
| ? oldDesc : { enumerable: true, get: function() { return m[k]; } }; |
Contributor
There was a problem hiding this comment.
I'm curious about the conditions here:
oldDesc.configurable === falsecould just be!oldDesc.configurable, becauseconfigurableis always set by the runtime.oldDesc.writable === falsemeans there won't be a"get"in oldDesc, becausewritableis only added for data properties (and isundefinedfor accessor properties).- There's a possibility that
oldDescis undefined (if re-exporting something from a dynamic module), in which caseoldDesc.writablecould throw
Would the following condition also match the intended behavior here?
Suggested change
| 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.
Contributor
There was a problem hiding this comment.
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);
Contributor
Author
There was a problem hiding this comment.
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). writablevsget-- 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
rbuckton
approved these changes
Jan 12, 2022
Contributor
|
Is there a PR for tslib yet? |
elibarzilay
added a commit
to microsoft/tslib
that referenced
this pull request
Jan 13, 2022
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.)
Contributor
Author
Made one now: microsoft/tslib#168 |
elibarzilay
added a commit
to microsoft/tslib
that referenced
this pull request
Jan 14, 2022
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.)
rbuckton
pushed a commit
to microsoft/tslib
that referenced
this pull request
Feb 8, 2022
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.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.