Skip to content
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

Compare fwienber/api-extractor-fix-3593-reference-alias with zelliott/refs + /exported #1

Draft
wants to merge 6 commits into
base: zelliott/integrate
Choose a base branch
from

Conversation

fwienber
Copy link
Owner

This PR just serves to see the diff to two PRs of @zelliott.

Maybe newlineKind: 'os' should be the default for api-extractor
config (ExtractorConfig.ts) anyway, but for now, let's make it
explicit in the test configuration.

Another possible scenario is that the tests are run under
Windows, but git has been configured to used 'lf'. Then, using
'os' line endings would still lead to a git diff. But detecting
the git setting is probably beyond what the test runner should
do.
"Incorrect canonical reference to aliased class in .api.json"
microsoft#3593

Fix first scenario from microsoft#3593.

Added test scenario "docReferencesAlias", inspired by
the test repo referenced in the GitHub issue:

`Item` and `Options` are default exports of two
separate source files, and `index` re-exports both,
renaming `Options` to `renamed_Options` (preserving
the "namespacing" information of the directory).

`DeclarationReferenceGenerator` now receives a function
to resolve a Symbol to its emit name.
`ApiModelGenerator` fetches this function from `Collector`,
which now maintains another map from symbol to
`CollectorEntity`. If for a given symbol, there is
a `CollectorEntity`, then its `emitName` is returned,
the symbol's name otherwise.

While this change fixes the `export { __ as __ }`
scenario, for _some_ reason, it changes the output
of one other test scenario, namely "ambientNameConflict".
In its `*.api.json` file, some reference to the
class re-exported as `MyPromise`, the name clash
of localFile's `Promise` and the global `Promise`
now leads to `Promise_2` being used instead of
`Promise`, the old expected result that was also
wrong, because it should have been `MyPromise`,
and `MyPromise` should appear in `*.api.json`, but
that is something completely different, so I'll
ignore the problem for now and check in the changed
expected `*.api.json` of "ambientNameConflict".
"Incorrect canonical reference to aliased class in .api.json"
microsoft#3593

This fixes the other aliasing, namely through (nested)
namespaces.
Added a test scenario "docReferencesNamespaceAlias", which
consists of nested namespaces that have their own index.d.ts,
re-exporting the default export of each sibling file and
their respective sub-namespace (top-level -> 'renamed'
-> 'sub').
The *.api.json canonicalReferences now correctly use the
nested namespace "fully-qualified name".

Again, another test result changed unexpectedly, maybe
revealing an existing bug / wrong expected result.
The scenario is "exportImportStarAs2", where some
'ForgottenClass' is now referenced via its 'forgottenNs'.
I hope that this is an improvement, but I am not sure
how "illegal" references to non-exported items are
supposed to behave.

Implementation:
---------------
When DeclarationReferenceGenerator#_getParentReference() finds
a parent symbol via the symbol tree, it must also resolve
namespace parent symbols the Collector derived from
'import * as ___'.
To that end, Collector now has a method getParentSymbols()
that pulls namespace imports from the CollectorEntities
of a namespace symbol (if present). This must happen in a
loop for nested namespaces.
Namespace imports are now also registered in the
_entitiesBySymbol Map, but I did not find the API to
access the symbol of the AstNamespaceImport astEntity
(ugly type assertion 'as unknown as ts.Type', which is
probably not even the right type to access 'symbol').
I found the reference while debugging, but could not
find it in any meaningful subtype.
Extract complex inline expression to apply several navigation
steps to a DeclarationReference into a utility method, adequately
called `_addNavigationSteps()`.
On the fly changed that when in `_getParentReference()`,
`parentRef` is undefined, instead of immediately returning
undefined, continue with the fallback logic.
@fwienber fwienber force-pushed the fwienber/api-extractor-fix-3593-reference-alias-rebased branch from e7e6f28 to d1bc27b Compare August 30, 2022 16:21
octogonz pushed a commit that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant