[api-extractor] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output #960

octogonz · 2018-11-27T06:55:39Z

This is based on the discussion in microsoft/tsdoc#7 .

Instead of the tsdocFlavor field in package.json:

{
  "name": "widget-lib",
  "version": "1.0.0",
  "main": "lib/index.d.ts",
  "typings": "lib/index.js",
  "tsdoc": {
    "tsdocFlavor": "AEDoc"
  }
}

...we now recognize that an external package is TSDoc-compatible by looking for a file dist/tsdoc-metadata.json with contents like this:

{
  "tsdocVersion": "0.12",
  "toolPackages": [
    {
       "packageName": "@microsoft/api-extractor",
       "packageVersion": "7.0.0"
    }
  ]
}

This is a prototype until an official implementation is provided by the @microsoft/tsdoc library.

…iles belonging to the current project (which may not have been built yet)

iclanton · 2018-11-27T09:16:16Z

apps/api-documenter/src/start.ts

-  __dirname, '..', FileConstants.PackageJson)
-);
-const myPackageJson: { version: string } = require(myPackageJsonFilename);
+const myPackageVersion: string = PackageJsonLookup.loadOwnPackageJson(__dirname, '..').version;


Do you actually need the ..? #Resolved

Yes, package.json is above the src folder.

We could walk upwards to probe for package.json, but that's inefficient and error-prone for a scenario that is supposed to be basically hardcoded.

In reply to: 236574942 [](ancestors = 236574942)

Actually I think you persuaded me

In reply to: 236587957 [](ancestors = 236587957,236574942)

iclanton · 2018-11-27T09:16:44Z

apps/api-extractor/src/extractor/Extractor.ts

+   * The NPM package version for the currently executing instance of the "\@microsoft/api-extractor" library.
+   */
+  public static get version(): string {
+    return PackageJsonLookup.loadOwnPackageJson(__dirname, '../..').version;


Same here. #ByDesign

iclanton · 2018-11-27T09:18:31Z

apps/api-extractor/src/generators/dtsRollup/AstSymbolTable.ts

-        } else {
-          throw new Error(`The specified entry point does not appear to have an associated package.json file:\n`
-            + sourceFile.fileName);
+      if (this._program.isSourceFileFromExternalLibrary(sourceFile)) {


Is this right, or is this condition backwards? #Resolved

It's right. If the source file is NOT from an external library, then it's part of the project we're analyzing, which obviously supports API Extractor.

In reply to: 236575683 [](ancestors = 236575683)

iclanton · 2018-11-27T09:19:44Z

apps/api-extractor/src/generators/dtsRollup/PackageMetadataManager.ts

+ * 2. The current project is not guaranteed to have a package.json file at all.  For example, API Extractor can
+ *    be invoked on a bare .d.ts file.
+ *
+ * Use ts.program.isSourceFileFromExternalLibrary() to test source files before passing the to PackageMetadataManager.


What would the perf impact be if we tested for this in PackageMetadataManager? #WontFix

Generally if you're inside PackageMetadataManager for a local file, it's already too late.

I could add an assertion here to catch that mistake however.

PackageMetadataManager is a fairly half-baked file right now, though. I want to make tsdoc-metadata.json a feature of the TSDoc library, and then we'll redesign PackageMetadataManager when we update it to use that.

In reply to: 236576224 [](ancestors = 236576224)

iclanton · 2018-11-27T09:20:35Z

apps/api-extractor/src/generators/dtsRollup/PackageMetadataManager.ts

+    // This feature is still being standardized: https://github.com/Microsoft/tsdoc/issues/7
+    // In the future we will use the @microsoft/tsdoc library to read this file.
+    const tsdocMetadataPath: string = path.join(packageJsonFolder,
+      'dist', PackageMetadataManager.tsdocMetadataFilename);


dist is an arbitrary assumption to make. This should probably be placed in the same directory as the rollup file. #Resolved

The rollup file may not exist. But we could put it in the same directory as the default entry point for the NPM package.

In reply to: 236576554 [](ancestors = 236576554)

I've updated TSDoc #7 to reflect this idea.

However I'm not going to implement it here, since this is a temporary workaround.

In reply to: 236597139 [](ancestors = 236597139,236576554)

iclanton · 2018-11-27T09:21:48Z

apps/api-extractor/src/start.ts

-  __dirname, '..', FileConstants.PackageJson)
-);
-const myPackageJson: { version: string } = require(myPackageJsonFilename);
+const myPackageVersion: string = PackageJsonLookup.loadOwnPackageJson(__dirname, '..').version;


This should just use Extractor.version #Resolved

iclanton · 2018-11-27T09:22:08Z

apps/rush-lib/src/api/Rush.ts

-    }
-
-    return Rush._version;
+    return PackageJsonLookup.loadOwnPackageJson(__dirname, '../..').version;


Same here with the ../.. #Resolved

iclanton · 2018-11-27T09:22:19Z

apps/rush/src/start.ts

@@ -51,7 +48,8 @@ import { MinimalRushConfiguration } from './MinimalRushConfiguration';

 // Load the configuration
 const configuration: MinimalRushConfiguration | undefined = MinimalRushConfiguration.loadFromDefaultLocation();
-const currentPackageJson: IPackageJson = JsonFile.load(path.join(__dirname, '..', FileConstants.PackageJson));
+
+const currentPackageVersion: string = PackageJsonLookup.loadOwnPackageJson(__dirname, '..').version;


And here #Resolved

iclanton · 2018-11-27T09:25:21Z

libraries/node-core-library/src/PackageJsonLookup.ts

+   * loading, an exception will be thrown instead.
+   */
+  public static loadOwnPackageJson(dirnameOfCaller: string, pathToPackageJsonFolder: string): IPackageJson {
+    const packageJsonPath: string = path.join(dirnameOfCaller, pathToPackageJsonFolder, FileConstants.PackageJson);


Seems like this would be simpler if it just used the normal package discovery algorithm. We could include an optimization where we check if the specified path is under a cached path, but without a node_modules folder between the provided path and the cached path. #Resolved

This needs to be a static member. That means we assume the files we look at will never change on disk, and the cache can accumulate entries without bound. We need to be careful about what we store in this cache.

In reply to: 236578450 [](ancestors = 236578450)

iclanton · 2018-11-27T09:26:57Z

apps/api-extractor/src/generators/dtsRollup/AstSymbolTable.ts

+            .tryGetPackageJsonFilePathFor(sourceFile.fileName);
+
+          if (packageJsonPath) {
+            throw new Error(`Please add a field such as "tsdoc": { "tsdocFlavor": "AEDoc" } to this file:\n`


Didn't we say we wanted to just use the metadata file if we're generating a rollup? #Resolved

Good catch -- I meant to delete this code

In reply to: 236579005 [](ancestors = 236579005)

iclanton

octogonz · 2018-11-28T00:56:51Z

Fixes #959

pgonzal added 7 commits November 26, 2018 21:57

Add PackageJsonLookup.loadOwnPackageJson() API to node-core-library

924dc88

Update various projects to use PackageJsonLookup.loadOwnPackageJson()

58a4300

Add Extractor.version API

6700652

Implement preliminary support for reading/writing tsdoc-metadata.json

1efb25c

Fix an issue where PackageMetadataManager was getting consulted for f…

51fd48b

…iles belonging to the current project (which may not have been built yet)

rush change

181a8e8

Improve logging

c28c261

iclanton reviewed Nov 27, 2018

View reviewed changes

iclanton approved these changes Nov 27, 2018

View reviewed changes

pgonzal added 3 commits November 27, 2018 16:47

PR feedback: eliminate pathToPackageJsonFolder parameter

e97a5f0

PR feedback

47db611

Remove obsolete code that was asking for "tsdocFlavor"

9c32be6

Merge branch 'master' into pgonzal/tsdoc-metadata

0fe4bf3

octogonz merged commit b31c2e7 into master Nov 28, 2018

octogonz changed the title ~~[api-extractor] Replace the old "tsdocFlavor" field with a dist/api-metadata.json output~~ [api-extractor] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output Nov 28, 2018

octogonz mentioned this pull request Nov 28, 2018

[api-extractor] Eliminate "tsdoc" field from package.json #959

Closed

octogonz deleted the pgonzal/tsdoc-metadata branch February 3, 2019 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-extractor] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output #960

[api-extractor] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output #960

octogonz commented Nov 27, 2018 •

edited

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

octogonz Nov 27, 2018

octogonz Nov 27, 2018

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

octogonz Nov 27, 2018

iclanton Nov 27, 2018 •

edited by octogonz

Loading

octogonz Nov 27, 2018

iclanton Nov 27, 2018 •

edited by octogonz

Loading

octogonz Nov 27, 2018

octogonz Nov 28, 2018

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

octogonz Nov 27, 2018

iclanton Nov 27, 2018 •

edited by octogonz

Loading

octogonz Nov 27, 2018

iclanton left a comment

octogonz commented Nov 28, 2018

[api-extractor] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output #960

[api-extractor] Replace the old "tsdocFlavor" field with a dist/tsdoc-metadata.json output #960

Conversation

octogonz commented Nov 27, 2018 • edited Loading

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton Nov 27, 2018 • edited by octogonz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iclanton left a comment

Choose a reason for hiding this comment

octogonz commented Nov 28, 2018

octogonz commented Nov 27, 2018 •

edited

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading

iclanton Nov 27, 2018 •

edited by octogonz

Loading