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] Declaration map rollup #1886

Open
1 of 2 tasks
kripod opened this issue May 25, 2020 · 18 comments
Open
1 of 2 tasks

[api-extractor] Declaration map rollup #1886

kripod opened this issue May 25, 2020 · 18 comments
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start!

Comments

@kripod
Copy link

kripod commented May 25, 2020

Is this a feature or a bug?

  • Feature
  • Bug

What is the expected behavior?

Similarly to how .d.ts files can be rolled up into a single bundle, I think that declaration maps may also be rolled up alongside them. Is it possible to do this with existing libraries like combine-source-map or sorcery?

@octogonz
Copy link
Collaborator

If API Extractor produces the rolled up .d.ts file, then API Extractor would need to produce the corresponding .d.ts.map file. (The combine-source-map or sorcery tools can be used to repackage existing .map files, but they cannot create a missing mapping.)

We could implement this feature for API Extractor, but I am curious how you would intend to use it.

@kripod
Copy link
Author

kripod commented May 26, 2020

Thank you for the answer, I would be very excited to have this feature built-in.

I’m working on a zero-config library bundler built on top of Rollup. It would transform TypeScript with Babel, while creating a rolled up declaration and declaration map with this API Extractor. (A cache would also possibly be kept using find-cache-dir for incremental TS builds.)

The entry points would be read from package.json as specified by the Node.js ESM docs. Configuration would be as simple as:

/* package.json */

{
  "main": "./dist/node-cjs/main.js",
  "exports": {
    ".": {
      source: "./src/main.ts",
      import: "./dist/node-esm/main.js",
      require: "./dist/node-cjs/main.js"
    },
    "./feature": {
      "source": "./src/feature.ts",
      "browser": "./feature/index.min.js", // Gets minified with Terser
      "default": "./feature/index.js"
    }
  }
}

@octogonz
Copy link
Collaborator

Interesting. Can you clarify why would it be useful to have a .d.ts.map for API Extractor's .d.ts rollup? What would you use that declaration map for?

@kripod
Copy link
Author

kripod commented May 26, 2020

Sure, thanks for asking. I would use @rollup/plugin-babel for transforming JS/TS code in general. As for generating and rolling up .d.ts files, the regular TypeScript compiler combined with API Extractor seems to be a perfect fit.

I would run Rollup (including Babel) in parallel with tsc and the API Extractor. At a later stage of the project, Rollup might be replaced with a faster tool like esbuild as it matures, leaving the extraction of type definitions intact.

@kripod
Copy link
Author

kripod commented May 26, 2020

I forgot to mention that original sources would be distributed with builds, so .d.ts.map files could point to the former ones. This would improve integration with editors like VS Code, making it possible to delve into a library's implementation with a click at imported resources.

@octogonz
Copy link
Collaborator

I see, so when a consumer imports your published NPM package, the compiler will use the .d.ts rollup. But when the developer presses F12 in VS Code ("go to definition"), you want it to navigate to the original src/**/*.ts source file, which will be included with the published NPM package. Is that right?

@kripod
Copy link
Author

kripod commented May 26, 2020

Exactly, that’s the plan.

@octogonz
Copy link
Collaborator

If API Extractor generates a .d.ts.map for the rollup, should the mapping should try to point to the original .ts file, NOT to the intermediary .d.ts file that the .d.ts rollup was generated from. Or would you prefer to solve that part using a separate tool such as sorcery.js?

@octogonz octogonz added the enhancement The issue is asking for a new feature or design change label May 26, 2020
@kripod
Copy link
Author

kripod commented May 26, 2020

I would prefer pointing to the original sources. So:

  1. Emit files with tsc.
  2. Roll up the emitted .d.ts files (along with their maps) into a single output.
  3. Make sure the emitted declaration maps point to the original source files, just like the intermediary separate .d.ts.map files did.

I don’t see a use-case for pointing back to tsc-emitted JS files instead of the original sources.

@octogonz
Copy link
Collaborator

Sounds like a plan. If someone wants to work on this, the existing code that uses .d.ts.map file to remap error messages is in SourceMapper.ts. This new feature will probably build upon that.

I may look at it myself if I find some free time. Seems like a fun little project.

@octogonz octogonz added effort: medium Needs a somewhat experienced developer help wanted If you're looking to contribute, this issue is a good place to start! labels May 27, 2020
@cymptom
Copy link

cymptom commented Dec 3, 2020

I'd love to see this feature. In my case, I'm using api-extractor to shape the public APIs exposed by packages only meant for internal consumption -- so it would make sense to always have things link back to the source code.

@Josmithr
Copy link
Contributor

I would love to see this too. This would be a huge win for IDE navigation.

@mistic
Copy link

mistic commented Oct 1, 2021

@octogonz have you found any time to work on this? We have a similar use case to the ones mentioned: we have a monorepo with internal packages that we transpile with tsc generating .d.ts and .d.ts.map files with a sourceRoot setting in place (in order to accommodate our current monorepo layout). Having that feature implemented will allow us to navigate into the sources within the IDE everytime we point and click in some import from those local packages. Is this something you think we can add as a feature in the api-extractor? I would love if we could prioritise this!

@octogonz
Copy link
Collaborator

octogonz commented Oct 5, 2021

I have not had time to work on this yet, or even to think about what's involved and what's the best approach. I do agree it's an important feature. I'll follow up with @mistic offline and see if we can at least scope out the work.

@scottmas
Copy link

scottmas commented Dec 9, 2021

For some context, this is extremely useful in monorepo projects when writing internal tools where you want developers to be able to step to the actual function definition from where they're consuming it.

@Zamiell
Copy link

Zamiell commented Aug 10, 2022

I just wanted to chime in with another use case for this feature.

I have a library written in TypeScript. It is consumed by users via npm (like you would expect).

In my library, I include both the source and the compiled output, along with the declaration map. That way, end-users can press F12 on the library functions to very-easily see what the resulting source code looks like. It's really good UX/DX!

Furthermore, the library has some internal functions that shouldn't be put into the auto-complete of end-users. Right now, I just have a crapload of @internal JSDoc comments, but this is pretty terrible. Thus, I would love to use the .d.ts rollup feature of api-extractor to automatically purge those. However, I can't do that without also breaking the declaration maps!

Thus, I am stuck and can't use api-extractor at all. Please consider prioritizing this feature addition, as I think my use-case is fairly ordinary, all things considered.

@ivancuric
Copy link

If I'm reading this right, rolling up declaration files is an antipattern according to the official docs:
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#packaging-dependent-declarations

If your type definitions depend on another package:

  • Don’t combine it with yours, keep each in their own file.
  • Don’t copy the declarations in your package either.
  • Do depend on the npm type declaration package if it doesn’t package its declaration files.

@octogonz
Copy link
Collaborator

octogonz commented Jul 26, 2023

If I'm reading this right, rolling up declaration files is an antipattern according to the official docs:
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#packaging-dependent-declarations

@ivancuric I think you're not reading it right. 🙂 The antipattern they're describing is embedding an external package's .d.ts files in your own package (for example, like what the bundledPackages feature supports). It creates trouble if a consumer of your NPM package may also import the bundled package, since now the TypeScript compiler has to process two copies of the same typings, which can produce confusion and type conflicts. This is normally NOT a problem for API Extractor's .d.ts rollups, because the unbundled .d.ts files are considered internal implementation details; nobody will import them. (And the bundledPackages feature is also okay, if it is used in situations where the bundled typings won't be imported in some other way, for example if you are the maintainer of both packages and can stipulate how they are imported.)

@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Feb 26, 2024
@iclanton iclanton moved this from Needs triage to AE/AD in Bug Triage Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Needs a somewhat experienced developer enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start!
Projects
Status: AE/AD
Development

No branches or pull requests

8 participants