refactor(compiler): Allow extraction for some private symbols, like $… #55340
Conversation
|
Deployed adev-preview for 886fb27 to: https://ng-dev-previews-fw--pr-angular-angular-55340-adev-prev-kl3pc92p.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
| const allowedPrivates = new Map([ | ||
| [`ɵ$localize`, '$localize'], // exported as private but available as global | ||
| ]); |
There was a problem hiding this comment.
nit: I'd propose renaming this Map to indicate that we are not trying to expose private symbols, it's just a mechanism by which we exported some symbols as globals:
| const allowedPrivates = new Map([ | |
| [`ɵ$localize`, '$localize'], // exported as private but available as global | |
| ]); | |
| const exportedAsGlobals = new Map([ | |
| [`ɵ$localize`, '$localize'], // exported as private but available as global | |
| ]); |
| @@ -23,6 +23,9 @@ import {extractTypeAlias} from './type_alias_extractor'; | |||
|
|
|||
| type DeclarationWithExportName = readonly[string, ts.Declaration]; | |||
|
|
|||
| const allowedPrivates = new Map([ | |||
There was a problem hiding this comment.
We have some precedent for global functions in core/global. Could we do something similar instead of special-casing $localize?
There was a problem hiding this comment.
This is what I tried first, there must be a subtle difference which makes that the symbol isn't picked up.
There was a problem hiding this comment.
@jelbourn might know more about it since he was doing the docs extraction.
There was a problem hiding this comment.
fwiw, my issue is/was that TypeChecker.getExportsOfModule doesn't pick up any of the re-exported symbols on a target created similarly than core/global
There was a problem hiding this comment.
Yeah, the docs extraction is currently just relying on TypeScriptReflectionHost.TypeScriptReflectionHost on the public-api files.
pkozlowski-opensource
added
the
area: compiler
…localize $localize is privately exported but defined globaly and is part of the public API. fixes angular#54388
JeanMeche
force-pushed
the
docs/ɵ$localize
branch
from
1f9e8fa
to
886fb27
Compare
| @@ -23,6 +23,9 @@ import {extractTypeAlias} from './type_alias_extractor'; | |||
|
|
|||
| type DeclarationWithExportName = readonly[string, ts.Declaration]; | |||
|
|
|||
| const exportedAsGlobals = new Map([ | |||
| [`ɵ$localize`, '$localize'], // exported as private but available as global | |||
| ]); | |||
There was a problem hiding this comment.
I think we need to clarify whether we want to hard-code this FW-repo specific logic into ngtsc/docs. My understanding is that the docs extraction can be a useful tool for other pipelines as well, in the future.
A more future-proof and also code-local solution would be adding special tag information to these exports. e.g. @forceApiGeneration
There was a problem hiding this comment.
Or idealy, understand why #55345 doesn't work ?
Having forceApiGeneration would still require us to rename the symbol to remove the bared o.
There was a problem hiding this comment.
Yeah, that would also work. In either case, not a big fan of a map in ngtsc/docs. The JSDoc could also include the name that it should be renamed to. e.g. @forceApiGeneration $localize
There was a problem hiding this comment.
It sounds like we should fix the root cause though
There was a problem hiding this comment.
FWIW there are already framework-specific patterns hard-coded into the docs extraction (though not as far as hard-coding specific symbols). If we ever want to open it up to code outside the Angular team, it would be a whole project to deal with this. I do agree, though, that it's better to avoid hard-coding things if we have a reasonable, more general alternative.
There was a problem hiding this comment.
Even aside from hard-coding specifics (while the current ones, as you said, are not symbol specific, except maybe Angular-ecosystem wide lifecycle hooks), keeping these "special behaviors" local to the exported declarations is cleaner and more obvious.
There was a problem hiding this comment.
I agree that if we can get #55345 to work, that would be better.
…localize
$localize is privately exported but defined globaly and is part of the public API.
fixes #54388