fix(ngcc): support static evaluation of exports object in CommonJS
#32424
Conversation
In CommonJS bundles, the `exports` object is available as global variable, without a `ts.Declaration` nor `ts.Expression` in the source file. During static interpretation of expressions that read from the `exports` object, ngtsc attempts to resolve the declaration node of the "exports" variable in order to determine the value it represents. As no such declaration exists, the evaluation results in a dynamic value. Since the `exports` variable represents the source file's exports, the source file itself can be used as declaration node for any "exports" variable in CommonJS bundles. Doing so allows ngtsc to evaluate its value correctly, as source file declarations are interpreted as the public exports of the source file.
JoostK
added
type: bug/fix
action: review
| // The "exports" object is available as global in CommonJS bundles, without a `ts.Declaration` | ||
| // nor `ts.Expression` in the source file. Since it represents the source file's exports, the | ||
| // source file itself can be used as declaration node. | ||
| if (id.text === 'exports') { |
There was a problem hiding this comment.
Do we need to be a bit more careful here? It is possible that the exports identifier is a local variable, no?
There was a problem hiding this comment.
@petebacondarwin Yes, that is correct. I was actually hoping that this statement could be the fallback check after the regular resolution had failed to resolve a declaration, however that didn't work out as I expected. The thing is, with the following code:
exports.abc = true;
The exports identifier will be resolved to a ts.Symbol with valueDeclaration corresponding with the exports.abc node. Therefore, the regular resolution would end up with a declaration for the identifier, but not the one we desire, preventing the fallback from being considered. I'm not sure if this is a bug in TypeScript or the way it's supposed to work.
We might be able to detect the above situation to be able to differentiate between local vars and the actual exports global, but I'm not really sure what would be the most accurate way.
|
This might not be necessary any more. As I was debugging NativeScript issues, I noticed that super.getDeclarationOfIdentifier(id) eventually returned the same thing ( (Not sure if there are other cases where this is still needed, but it worked for my usecase.) |
petebacondarwin
added
target: patch
|
@JoostK - can you determine if this is still necessary and cleanup or close this PR as necessary? |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
In CommonJS bundles, the
exportsobject is available as globalvariable, without a
ts.Declarationnorts.Expressionin the sourcefile. During static interpretation of expressions that read from the
exportsobject, ngtsc attempts to resolve the declaration node of the"exports" variable in order to determine the value it represents. As no
such declaration exists, the evaluation results in a dynamic value.
Since the
exportsvariable represents the source file's exports, thesource file itself can be used as declaration node for any "exports"
variable in CommonJS bundles. Doing so allows ngtsc to evaluate its
value correctly, as source file declarations are interpreted as the
public exports of the source file.