Skip to content

[api-extractor] Support for import() type #1916

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

Conversation

javier-garcia-meteologica
Copy link
Contributor

@javier-garcia-meteologica javier-garcia-meteologica commented Jun 5, 2020

Provides a solution for #1050.

Transforms

    bar: import('foo').bar;

into

import * as _ImportType from 'foo';
...
    bar: _ImportType.bar;

It also contains a workaround for jsdom/jsdom#2304

  "jest": {
    "testURL": "http://localhost/"
  }

EDIT: The jest issue went away after doing a git rebase

@octogonz
Copy link
Collaborator

octogonz commented Jun 9, 2020

Sorry for the delay - I will look at this PR soon.

@octogonz
Copy link
Collaborator

octogonz commented Jun 9, 2020

FYI we recently reformatted the master branch using Prettier, which creates merge conflicts for existing PRs. We'll try to fix up the existing PRs for people, but if you want to do it yourself, here's the steps:

  1. Run "npm install -g [email protected]" to install Prettier.
  2. Merge the "release/master-before-prettier" branch into your PR branch. This is the state of the world before Prettier was applied.
  3. Merge the "release/master-after-prettier" branch into your PR branch. This will create a merge conflict with the Prettier changes, and nothing else.
  4. Resolve all the merge conflicts by discarding "origin", and overwriting with your branch's changes.
  5. BEFORE committing the merge, run "prettier . --write" from the repo root to reapply the formatting (that you discarded in step 4).
  6. Make sure to restage everything.
  7. Commit the merge
  8. Now you should be able to merge from the real "master" without any (Prettier-related) merge conflicts

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the import_type_support_simplicity branch 4 times, most recently from d052f14 to 2c7d763 Compare June 11, 2020 11:52
@javier-garcia-meteologica
Copy link
Contributor Author

Done

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the import_type_support_simplicity branch 2 times, most recently from ba73107 to 6ed9c43 Compare June 26, 2020 14:17
@javier-garcia-meteologica javier-garcia-meteologica changed the title Support for import() type [api-extractor] Support for import() type Jun 26, 2020
@adventure-yunfei
Copy link
Contributor

This may not work for some regular case, e.g.

// node_modules/foo/index.d.ts
export declare class Foo {}

// src/bar.d.ts
export { Foo } from 'foo';

// src/index.d.ts
export declare class A {
  prop: import('./bar').Foo;
}

In this case, it's not external import (detected here), and not a local symbol (it's defined inside external foo package). But here we treat it as a local symbol and so extract its detail definition.

The result is, declaration Foo will be included in the dts rollup result, which is not correct:

// dts-rollup.d.ts
export declare class Foo {}
export declare class A {
  prop: Foo;
}

This is the similar case that I met when trying to use import A = B.C.D; (#2005).
To fix that, the correct way (maybe) is trying to extract the external import recursively from the qualifier. (e.g. for import('./a').B.C.D, try extracting B, and then C, and D)

@adventure-yunfei
Copy link
Contributor

In my implementation adventure-yunfei#4 trying to resolve #2005, I introduced "exportPath" on "exportName" for AstImport, to hide details after analyzing phase.

Analyzer always analyze the whole import()....; declaration. For examples:

  • for import('a').B.C.D;, analyzer is saying "importing the content from module "a" with path ".B.C.D""
  • for import('./a').B.C.D;, if it refers to a local symbol, analyzer is saying "referring to the local symbol "D""
  • for import('./a').B.C.D; if it refers to an external symbol, and the latest external import (starting from the left) is symbol "C" with module path "modC", analyzer is saying "importing content from the module "modC" with path ".D"

That may decouple the generator from the analyzer.

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the import_type_support_simplicity branch from d7c526e to d73c4f8 Compare July 15, 2020 14:04
@javier-garcia-meteologica
Copy link
Contributor Author

There are new changes in my branch, but I guess something wrong with github because the changes are not showing up in this pull request. My branch has sha1 faa861a here, but this PR is stuck on d7c526e.

@adventure-yunfei, I like your plans for decoupling the analyzer from the generator. Now I've implemented a new AstImportKind.ImportType for import() types. For the moment I just store the full qualifier under exportName, but the code will become cleaner after exportPath is implemented.

Symbols now are resolved recursively, checking at each iteration if they proceed from an external module. I've replicated your example in api-extractor-scenarios and it's rolled up fine.

@javier-garcia-meteologica javier-garcia-meteologica force-pushed the import_type_support_simplicity branch 3 times, most recently from 0350dd7 to 829600a Compare July 15, 2020 16:29
@javier-garcia-meteologica javier-garcia-meteologica force-pushed the import_type_support_simplicity branch from d3a312e to 51e3fcc Compare June 10, 2021 11:14
@javier-garcia-meteologica javier-garcia-meteologica force-pushed the import_type_support_simplicity branch from a558cb7 to e2a2d08 Compare June 10, 2021 13:37
@javier-garcia-meteologica
Copy link
Contributor Author

@JamesBurnside, I have investigated the problem that you encountered and I believe it should be solved with the commit that I just pushed.

The problem is that the modified ImportType is missing the separator after. First I tried removing .skipAll() from the ImportType Span and only replacing prefix and omitting children. It didn't work. I had to resort using a regex to capture trailing spaces on the original text, and inserting them later on.

Finally, I made another change. Now DtsEmitHelpers has a function to modify ImportType that is shared between api report and dts generators.

@JamesBurnside
Copy link
Member

@JamesBurnside James Burnside FTE, I have investigated the problem that you encountered and I believe it should be solved with the commit that I just pushed.

The problem is that the modified ImportType is missing the separator after. First I tried removing .skipAll() from the ImportType Span and only replacing prefix and omitting children. It didn't work. I had to resort using a regex to capture trailing spaces on the original text, and inserting them later on.

Finally, I made another change. Now DtsEmitHelpers has a function to modify ImportType that is shared between api report and dts generators.

Just tested this in my repo and yep this fixed the spacing issue!

octogonz added 3 commits July 1, 2021 17:49
…_support_simplicity

# Conflicts:
#	apps/api-extractor/src/analyzer/AstImport.ts
#	build-tests/install-test-workspace/workspace/common/pnpm-lock.yaml
octogonz added 3 commits July 1, 2021 19:11
…_support_simplicity

# Conflicts:
#	common/config/rush/pnpm-lock.yaml
#	common/config/rush/repo-state.json
@octogonz
Copy link
Collaborator

octogonz commented Jul 2, 2021

I have resolved the conflicts from PR 1796. I want to read over the code once more before we merge this, but overall this looks to be in good shape. 👍

@benjdlambert
Copy link

@octogonz what do you think? Can we get a 🚢 ? 🙏

@octogonz octogonz enabled auto-merge July 8, 2021 05:21
@octogonz octogonz merged commit a6ce57a into microsoft:master Jul 8, 2021
@octogonz
Copy link
Collaborator

octogonz commented Jul 8, 2021

🎈Released with API Extractor 7.18.0

@octogonz
Copy link
Collaborator

octogonz commented Jul 8, 2021

@javier-garcia-meteologica thanks again for contributing this difficult feature! I am so happy to finally be getting through the backlog of AE PRs -- it's been keeping me awake at night for the past year. 😁

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.

8 participants