Skip to content

Conversation

@rbuckton
Copy link

@rbuckton rbuckton commented Sep 22, 2019

This builds on #1536 and adds support for including nested namespaces in the YAML output file.

NOTE: This is not a solution in and of itself, as it requires a separate change to the default DocFX templates to properly support namespaces. That PR is forthcoming.

The DocFX PR can be found here: dotnet/docfx#5128

@rbuckton
Copy link
Author

I've added a few other things to this PR:

  • Comprehensive IYamlApiFile documentation and a number of updates to typescript.schema.json to indicate all of the YAML properties known to DocFX
  • Include the inheritance hierarchy when available to match the implementation in DocFX
  • Adds Type Aliases and Variables to the YAML output, as I plan to also include them in the PR for DocFX to add proper support for namespaces.

@rbuckton
Copy link
Author

Here is a preview of the DocFX template changes I plan to propose:

image

image

@rbuckton
Copy link
Author

The related DocFX PR can be found here: dotnet/docfx#5128

@rbuckton rbuckton changed the title Include namespaces in YAML output [api-documenter] Include namespaces in YAML output Sep 25, 2019
@octogonz octogonz added the blocked We can't proceed because we're waiting for something label Sep 25, 2019
@octogonz
Copy link
Collaborator

If I understand right, we can't accept this PR until the DocFX PR dotnet/docfx#5128 is merged and released.

@rbuckton
Copy link
Author

rbuckton commented Oct 12, 2019

This dotnet/docfx#5128 was shipped in DocFX 2.46 last night. Per an offline conversation with @octogonz, we should probably consider putting this behavior behind a flag (or possibly putting the legacy behavior behind a flag) before merging.

Clarification: "This" refers to dotnet/docfx#5128, not this PR.

@octogonz octogonz removed the blocked We can't proceed because we're waiting for something label Oct 12, 2019
@octogonz
Copy link
Collaborator

dotnet/docfx#5128 was shipped in DocFX 2.46 last night

@AlexJerabek The Office Add-ins API has fairly extensive usage of TypeScript namespaces.
Could you test this API Documenter PR branch with your docs? (And if it's not working, maybe confirm whether your DocFX service has version >= 2.46 yet.)

@AlexJerabek
Copy link
Contributor

@octogonz This seems to radically change our reference docs. (Internal Staging Site link here. It looks like most of the content is gone.

I can dig deeper to learn what's happening and whether it's our internal tooling pipeline. I might need some guidance from @rbuckton.

That said, this will be challenging timing with Ignite on the horizon. What is the timeline for this update's release?

@octogonz
Copy link
Collaborator

octogonz commented Oct 16, 2019

This seems to radically change our reference docs. (Internal Staging Site link here.

I don't have permissions to view that site. (Although it might be super helpful if I did... 😉)

It looks like most of the content is gone.

Could you confirm whether you're using DocFX 2.46? My understanding:

Prior to this PR, namespaces were faked by (1) flattening the nested objects to be top-level objects with dotted names, and (2) simulating the nesting in the table of contents.

After this PR, nesting namespaces are now accurately represented in the .yml files. This required updating the DocFX template to render them properly. With the old template, the nested objects are not recognized and get ignored, which may be what caused them to disappear.

What is the timeline for this update's release?

@rbuckton's PR has been open for a while, so we're eager to finally wrap it up. But we definitely would not merge this PR without sign-off from your group.

@rbuckton
Copy link
Author

@AlexJerabek I assume you're using customized DocFX templates? It could be a number of things, including:

  • Differences in how UIDs are generated if you are using DocFX overwrite files:

    • In [api-extractor] Deeply linked types #1337 we updated the UID format to one that is more stable and resilient to collisions (i.e. two declarations of different types with the same name, such as a class and an interface). If you are using overwrite files, the YAML frontmatter may have a uid: field that no longer matches the generated UID.
    • If you are constructing a custom TOC and using UIDs to link items, they may no longer match.
  • Differences between the custom DocFX templates vs the changes in Improves the default template for TypeScript documentation dotnet/docfx#5128

  • Prior to this PR, namespaces were completely removed from the output YAML, i.e:

    namespace Foo {
      class Bar {}
    }

    Would have previously been represented in the TOC as:

    • Foo.Bar

    But now will be represented in the TOC as:

    • Foo
      • Bar

    Also, namespaces can have their own page.

@rbuckton
Copy link
Author

If the sources are the ones at https://github.com/OfficeDev/office-js-docs-reference, I can look into the differences later this week.

@AlexJerabek
Copy link
Contributor

@rbuckton Correct, our docs are hosted on https://github.com/OfficeDev/office-js-docs-reference. I'm testing these changes on the AlexJ-RSTest branch.

I'll send some time today figuring out if the issue is with our post-processing scripts. I don't want this to block your progress.

@AlexJerabek
Copy link
Contributor

One unrelated note that this change highlights:
The docs.microsoft.com platform does not allow TOC folder nodes to have content. There are accessibility issues with that. Those pages are automatically are automatically moved into that folder as a page with the same name.
image
As you can see, the namespace pages are copied into their own folder. Perhaps there's a way to account for this in the naming conventions.

@octogonz
Copy link
Collaborator

As you can see, the namespace pages are copied into their own folder. Perhaps there's a way to account for this in the naming conventions.

FYI the Rush Stack table of contents has a similar design, using a "(members)" node. For example:

Capture

The motivation was also for accessibility -- clicking on the container node should expand/collapse the children, rather than navigating to a page.

@octogonz
Copy link
Collaborator

@AlexJerabek any progress with this? According to microsoft/TypeScript-Website#80, the TypeScript compiler wants to adopt API Extractor for their docs (🎉yeah!), but they're blocked behind this PR. If you are busy with Ignite, is there a way we could get someone else to help with the validation?

@AlexJerabek
Copy link
Contributor

I was really hoping @rbuckton could lend some insight into the changes and help debug from our end. I'm not 100% what you need from us to help debug.

We can always use an older version of the API Extractor/Documenter if need be.

@rbuckton
Copy link
Author

My apologies, I've been swamped with other work and haven't been able to look further into this just yet. I will look into it this weekend.

@rbuckton
Copy link
Author

I've packed up a version of api-documenter from PR into a tarball so I can more easily test it on the office docs build: microsoft-api-documenter-7.4.6.tar.gz

@rbuckton
Copy link
Author

Other related packs:
microsoft-api-extractor-model-7.4.2.tar.gz
microsoft-api-extractor-7.4.4.tar.gz

@octogonz
Copy link
Collaborator

octogonz commented Nov 4, 2019

What's the next step? Are we waiting for @AlexJerabek to do something here?

@rbuckton
Copy link
Author

rbuckton commented Nov 5, 2019

I'm doing some additional investigation on my end right now.

@rbuckton rbuckton requested a review from D4N14L as a code owner November 15, 2019 22:48
@rbuckton
Copy link
Author

I've talked with @AlexJerabek offline and we've broken this down into three action items:

  1. Fix "Package" rendering for the docs.microsoft.com templates [PR] - @rbuckton
  2. Update this PR to include a --namespaces parameter for the yaml command (and also a "documentNamespaces" configuration property for the generate command) to opt-in to the new behavior (the old behavior for namespaces will remain the default). - @rbuckton
  3. Update the postprocessor script Office uses where necessary. - @AlexJerabek

@octogonz: (1) is not related to this change, and (2) has been completed. If this seems like an acceptable solution, then it is possible this PR is ready to merge.

# Conflicts:
#	apps/api-documenter/src/documenters/IConfigFile.ts
#	apps/api-documenter/src/schemas/api-documenter.schema.json
#	build-tests/api-documenter-test/config/api-documenter.json
@octogonz
Copy link
Collaborator

That's really great! I'll look over the final changes asap. Hopefully we can get it merged over the weekend.

Update this PR to include a --namespaces parameter for the yaml command (and also a "documentNamespaces" configuration property for the generate command) to opt-in to the new behavior

@rbuckton Once this new behavior has been rolled out and vetted, we will want to remove the old behavior and then remove this "opt-in" switch, right? What's the expected timeframe/criteria for that?

@rbuckton
Copy link
Author

Once this new behavior has been rolled out and vetted, we will want to remove the old behavior and then remove this "opt-in" switch, right? What's the expected timeframe/criteria for that?

I'd leave that up to you. If I needed to implement a phased rollout for this I might suggest the following:

  1. Release with the opt-in behavior for now. This gives you a chance to collect feedback on the design and work out any possible issues the new format might introduce.
  2. Following that release, introduce a --no-namespaces flag (which is still the default behavior) and print a warning message if you use the command line without at least one of --namespaces or --no-namespaces set. Also print a warning message if using generate and the documentNamespaces option is unset.
  3. (Optional) Following that release, make the warning into an error.
  4. Finally, switch the default and remove the warning/error.

It all depends on how long you want to phase in the behavior, or whether you want to phase it in at all. I will likely run with --namespaces set, but there's no telling how many others would prefer the existing behavior.

@octogonz
Copy link
Collaborator

Hmm.. I assumed the new behavior was clearly superior to the old one. In what situation would someone prefer the old behavior?

BTW DocFx's philosophy was "one best way" rather than giving documentation authors lots of control over how their API gets modeled. The idea being that APIs are all made from the same essential building blocks, whereas the website designers should be free to improve the presentation over time.

@rbuckton
Copy link
Author

Hmm.. I assumed the new behavior was clearly superior to the old one. In what situation would someone prefer the old behavior?

BTW DocFx's philosophy was "one best way" rather than giving documentation authors lots of control over how their API gets modeled. The idea being that APIs are all made from the same essential building blocks, whereas the website designers should be free to improve the presentation over time.

I could always change the flag to be --no-namespaces so that the default behavior is the new behavior, with the opt-out flag as an option for anyone that needs to migrate, but that would be a breaking change for anyone not expecting it (possibly indicating the need to increment the major version number).

@octogonz
Copy link
Collaborator

It would be preferable to make it "opt-in" for the current major version (i.e. while we validate that it works), and then completely remove the old behavior when we release the next major version (i.e. ship it with confidence).

My main question: Is there any legitimate reason why someone would NOT want to adopt the new behavior? The old behavior seemed pretty hacky, so I'd really prefer to have a single solution for how we document namespaces.

@octogonz
Copy link
Collaborator

I finally got some time to review the PR. Overall it looks good.

My only suggestion is that we should make the setting name consistent between the CLI and api-documenter.json file, and the name should better communicate that it's an "opt in".

So I'm thinking we'll rename it to --new-docfx-namespaces. I can make this change. I'll also add a note saying that this is will be officially enabled in the next major release.

BTW do you think this would be relevant to the Markdown renderer? The way it renders right now actually looks okay to me, although I haven't used namespaces very much.

…lear that this is an opt-in for an experimental DocFX-specific feature
@octogonz octogonz merged commit 625c095 into microsoft:master Nov 24, 2019
@octogonz octogonz deleted the flattenNamespaces2 branch November 24, 2019 00:28
@octogonz
Copy link
Collaborator

octogonz commented Nov 24, 2019

@rbuckton Thanks again for doing this work! 😄 We published your change with API Documenter 7.7.0.

rikhoffbauer pushed a commit to rikhoffbauer/microsoft__api-documenter that referenced this pull request Mar 23, 2020
[api-documenter] Include namespaces in YAML output
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.

3 participants