[api-extractor] Add handling of file reference directives by willmtemple · Pull Request #1955 · microsoft/rushstack

willmtemple · 2020-06-22T22:23:55Z

This is a first stab at supporting path-based references like:

/// <reference path="path/to/xyz.d.ts" />

in API Extractor.

Would fix: #1761 (CC @jeremymeng)

We use these references as part of a shimming pattern in the Azure SDK for JavaScript, but we aren't able to use dtsRollup for packages that need them because d.ts rollup currently just doesn't handle them.

Because these references are relative to the SourceFile that they are collected from, the path that is emitted has to be corrected to be relative to the ultimate file. I've written the logic for this as internal functions next to the Collector and DtsRollupGenerator, but could relocate them as needed.

I have not tested this on Windows yet.

octogonz · 2020-06-23T09:57:34Z

apps/api-extractor/src/collector/Collector.ts

+/**
+ * Resolve the name of a file to an absolute path relative to a TypeScript SourceFile node.
+ */
+function resolveNameRelativeToSourceFile(name: string, source: ts.SourceFile): string {


Can you simply use path.relative()?

For this one, I don't think so. I need to boot into Windows and check it, but I believe that the reference directive in text has to follow the POSIX convention and use forward slash as its path separator just like a node module, but the file name of the source file could be in Windows convention, so relying on path to process the reference path may not be safe. Need to do some testing.

If the input and output paths are POSIX format, then you can use path.posix.relative().

If the input path is Windows style and you want to convert to POSIX, there is no theoretically correct API for that, but Text.replaceAll(thePath, '\\', '/') works reasonably well with Node.js

octogonz · 2020-06-23T09:59:51Z

apps/api-extractor/src/generators/DtsRollupGenerator.ts

+/**
+ * Compute a relative path between two absolute paths.
+ */
+function computeRelativePath(fromPath: string, toPath: string): string {


Can you simply use path.relative()?

In this instance, sure. The Path module in the node-core-library package in this repo made me think that there was some reason for avoiding it.

FileSystem is a complete replacement for fs, because the native fs library was designed as low-level primitives that are not suitable for everyday usage.

Path contains supplementary stuff that merely extends the native path, since that API is fine to use.

We should probably document this better.

octogonz · 2020-06-23T10:05:33Z

Thanks for making a PR! 😁

Could you add a test case to the rushstack\build-tests\api-extractor-scenarios folder? (The test cases get registered in rushstack\build-tests\api-extractor-scenarios\config\build-config.json.)

octogonz · 2020-06-23T10:14:28Z

apps/api-extractor/src/collector/Collector.ts

+   * /// <reference path="runtime-library" />
+   * ```
+   */
+  public get dtsFileReferenceDirectives(): ReadonlySet<string> {


The runtime-library example is not intended to be a path. Maybe something like this:

/** * A list of paths (e.g. "path/to/file.d.ts") that should appear in a reference like this: * * ``` * /// <reference path="path/to/file.d.ts" /> * ``` * * The paths are resolved relative to the source file containing the reference. */

ah yeah just missed this when copying the doc string 👍

willmtemple · 2020-06-25T20:38:37Z

@octogonz I am getting a build failure on Windows in node-core-library. blocking api-extractor-scenarios from building.

I'm just building using rush update then rush build in Windows PowerShell, and I've set my execution policy to "Bypass" for the process. I don't know if there are other special instructions for building on windows because I can't find documentation on the build process.

Here is the error:

 FAIL  lib\test\Executable.test.js
  ● Executable.spawnSync("npm-binary-wrapper") edge cases 1

    Error

      86 |     expect(result.error).toBeUndefined();
      87 |     expect(result.stderr).toBeDefined();
    > 88 |     expect(result.stderr.toString()).toEqual('');
      89 |     expect(result.stdout).toBeDefined();
      90 |     const outputLines = result.stdout
      91 |         .toString()
      Error: expect(received).toEqual(expected)

      Expected value to equal:
        ""
      Received:
        "The directory name is invalid.
      "
      at executeNpmBinaryWrapper (lib/test/Executable.test.js:88:38)
      at Object.<anonymous> (lib/test/Executable.test.js:119:12)

  ● Executable.spawnSync("npm-binary-wrapper") edge cases 2

    Error

      86 |     expect(result.error).toBeUndefined();
      87 |     expect(result.stderr).toBeDefined();
    > 88 |     expect(result.stderr.toString()).toEqual('');
      89 |     expect(result.stdout).toBeDefined();
      90 |     const outputLines = result.stdout
      91 |         .toString()
      Error: expect(received).toEqual(expected)

      Expected value to equal:
        ""
      Received:
        "The system cannot find the path specified.
      "
      at executeNpmBinaryWrapper (lib/test/Executable.test.js:88:38)
      at Object.<anonymous> (lib/test/Executable.test.js:129:12)

  ● Executable.spawnSync("npm-binary-wrapper") edge cases 2

    Error

      86 |     expect(result.error).toBeUndefined();
      87 |     expect(result.stderr).toBeDefined();
    > 88 |     expect(result.stderr.toString()).toEqual('');
      89 |     expect(result.stdout).toBeDefined();
      90 |     const outputLines = result.stdout
      91 |         .toString()
      Error: expect(received).toEqual(expected)

      Expected value to equal:
        ""
      Received:
        "The system cannot find the path specified.
      "
      at executeNpmBinaryWrapper (lib/test/Executable.test.js:88:38)
      at Object.<anonymous> (lib/test/Executable.test.js:139:12)

 PASS  lib\test\PackageName.test.js
 PASS  lib\test\Text.test.js
 PASS  lib\test\ProtectableMap.test.js
 PASS  lib\test\PackageJsonLookup.test.js
 PASS  lib\test\Path.test.js
 PASS  lib\test\Sort.test.js
 PASS  lib\test\JsonSchema.test.js
 PASS  lib\test\FileSystem.test.js
[13:20:24] Error - 'jest' sub task errored after 8.75 s
 Jest tests failed
[13:20:24] 'test' errored after 18 s
[13:20:24]
About to exit with code: 1
Process terminated before summary could be written, possible error in async code not continuing!
Trying to exit with exit code 1
The script failed with exit code 1

octogonz · 2020-06-26T05:19:08Z

I am getting a build failure on Windows in node-core-library. blocking api-extractor-scenarios from building.

I'm just building using rush update then rush build in Windows PowerShell, and I've set my execution policy to "Bypass" for the process. I don't know if there are other special instructions for building on windows because I can't find documentation on the build process.

@willmtemple I cannot repro this.

What is the full path of your repo (pwd in PowerShell)? I wonder if one of the parent paths has weird characters, or exceeds MAX_PATH. I typically clone my repo as C:\Git\rushstack to minimize these issues.

octogonz · 2020-07-08T01:01:51Z

I don't know if there are other special instructions for building on windows because I can't find documentation on the build process.

API Extractor docs are here. General repo instructions are here.

octogonz · 2020-08-28T07:45:41Z

@willmtemple Following up -- are you still working on this PR? Do you need anything else from us?

willmtemple · 2020-08-31T16:14:34Z

@octogonz Had to put this down for a while. The build issue I was running into was indeed a MAX_PATH issue. This week I'm going to revisit this. Referring to your comment in the linked issue:

For a rollup, there's some question as to how those paths should be handled.

Do you have any thoughts on the two approaches to take here? This implementation is leaving the file references intact in the output bundle, just rewriting them to be relative to the output location, but the other alternative of course is that we could include the referenced file in the rolled-up d.ts.

pd4d10 · 2022-02-06T13:59:50Z

This PR also seems to fix #3000

Is there anything I can do to help?

[api-extractor] Add handling of file reference directives

e16800e

willmtemple requested review from D4N14L, apostolisms, halfnibble, iclanton, octogonz and sachinjoseph as code owners June 22, 2020 22:23

octogonz reviewed Jun 23, 2020

View reviewed changes

Add test, use path primitives

b9969ed

willmtemple mentioned this pull request Jun 23, 2020

[KeyVault] tsconfig.json feedback by Will Azure/azure-sdk-for-js#9616

Closed

octogonz added the PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. label Aug 28, 2020

octogonz requested a review from dmichon-msft as a code owner April 22, 2023 19:03

Conversation

willmtemple commented Jun 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octogonz commented Jun 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willmtemple commented Jun 25, 2020

Uh oh!

octogonz commented Jun 26, 2020

Uh oh!

octogonz commented Jul 8, 2020

Uh oh!

octogonz commented Aug 28, 2020

Uh oh!

willmtemple commented Aug 31, 2020

Uh oh!

pd4d10 commented Feb 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants