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

[api-extractor] add basic support for import * as module from './local-module' #1796

Merged
merged 39 commits into from
Jun 30, 2021

Conversation

mckn
Copy link
Contributor

@mckn mckn commented Mar 25, 2020

I have basically taken the PR created by @adventure-yunfei here PR and cleaned up the code a bit. Tested it on the Grafana project which is a really big project and then opened this PR.

Thanks and great work by @adventure-yunfei!

Please let me know what you want to be adressed to get it merged. I think there are several people that wants to help out to get this PR merged.

Fixes: #1029

@msftclas
Copy link

msftclas commented Mar 25, 2020

CLA assistant check
All CLA requirements met.

@mckn mckn changed the title So this PR adds basic support for import * as module from './local-module' [api-extractor] add basic support for import * as module from './local-module' Mar 25, 2020
@mckn
Copy link
Contributor Author

mckn commented Mar 25, 2020

I can run the tests and see that there are two failing tests but not sure how to get some more details about the failure.

Saw that it was a build failure and got more details by running the regular gulp script in the api-extractor.

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had two style/practice comments.

I ran into this issue today and am looking forward to these changes, thanks @mckn!

apps/api-extractor/src/analyzer/ExportAnalyzer.ts Outdated Show resolved Hide resolved
apps/api-extractor/src/generators/ApiReportGenerator.ts Outdated Show resolved Hide resolved
@stevengum
Copy link
Member

@octogonz sorry for the out-of-the-blue PR review, just excited to use api-extractor on my team's projects 😄

@mckn, do you have tests that can be added to the PR?

@mckn
Copy link
Contributor Author

mckn commented Mar 27, 2020

@octogonz sorry for the out-of-the-blue PR review, just excited to use api-extractor on my team's projects 😄

@mckn, do you have tests that can be added to the PR?

@stevengum awesome feedback! Thanks for doing the review. Have made the updates and pushed the changes.

I have not added any tests but please give me some directions on how I can write some tests and I am happy to add that as well.

@stevengum
Copy link
Member

@mckn sorry for the delay, I'm not a regular contributor to this project so I don't know how to write/run tests.

I've pulled the grafana branch and was going to run the tests locally, it looks like they've already passed as part of the setup of the repo.

I don't have insight into their CI pipelines either, could the rushstack CI Build be stuck in pending due to the merge conflict?

@mckn
Copy link
Contributor Author

mckn commented Apr 1, 2020

@stevengum I have added one some simple tests to verify the import * as .. scenario. Not sure if it is the merge conflict that is causing errors. But I will sync with upstream asap so we can get this mergeable.

@mckn
Copy link
Contributor Author

mckn commented Apr 3, 2020

@octogonz would be awesome if you could have a look at this PR 🙏

@rbuckton
Copy link
Member

rbuckton commented Apr 4, 2020

I tried this on one of my projects and the links in the TOC for namespaces are broken (as in, clicking on them does nothing).

This is when using the YamlDocumenter API via new YamlDocumenter(apiModel, /*newDocfxNamespaces*/ true);.

@rbuckton
Copy link
Member

rbuckton commented Apr 4, 2020

Disregard that comment. After a clean build of my documentation it seems to be working fine.

@santam85
Copy link

Happy to see there's a PR already in this advanced stage for this. This is a must to support api-extractor for Angular projects as well, as there's limited control on the typings output from the CLI

@lifeiscontent
Copy link

@octogonz any updates here?

@octogonz
Copy link
Collaborator

@octogonz any updates here?

To be clear, what's blocking this PR wasn't the code review, but rather some nontrivial work required to complete the implementation. (Some of which is identified by TODO in the author's comments.) I'm looking at it again this weekend to see if I can push it along.

@octogonz
Copy link
Collaborator

Today I got the .api.md file straightened out. The last step will be the .api.json file.

@octogonz
Copy link
Collaborator

🚀 This is finally ready to go!

I tested it by building a large production monorepo with lots of projects that use API Extractor.

@octogonz octogonz merged commit 3ade23f into microsoft:master Jun 30, 2021
@octogonz octogonz deleted the export-star-as-support branch June 30, 2021 06:12
@octogonz octogonz restored the export-star-as-support branch June 30, 2021 06:12
@octogonz
Copy link
Collaborator

Released with API Extractor 7.17.0

Qjuh pushed a commit to discordjs/rushstack that referenced this pull request Nov 7, 2023
microsoft#1796 (comment)

# Conflicts:
#	apps/api-extractor/src/analyzer/AstSymbolTable.ts
#	apps/api-extractor/src/analyzer/ExportAnalyzer.ts
#	apps/api-extractor/src/collector/Collector.ts
#	apps/api-extractor/src/generators/ApiModelGenerator.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[api-extractor] Support "import * as __ from __" for local paths
10 participants