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

Declaration maps do not always work #117

Open
domoritz opened this issue Sep 7, 2020 · 5 comments
Open

Declaration maps do not always work #117

domoritz opened this issue Sep 7, 2020 · 5 comments
Labels
Bug Something isn't working

Comments

@domoritz
Copy link
Contributor

domoritz commented Sep 7, 2020

  • Version: 1.3.4
  • Rollup Version: 2.26.10
  • Operating System and version (if applicable): macOS
  • Node Version (if applicable): 14.9.0
  • Does it work with tsc (if applicable): yes

I might be missing something obvious but unfortunately, declaration maps stopped working for me when I switched to this plugin.

Reproduction

Check out https://github.com/vega/vega-tooltip/tree/fee07c92ce6b1914650a0daf881ef403249e6374 and run yarn && yarn build. Then link from another js module and run

import * as tooltip from 'vega-tooltip';

tooltip.formatValue;

Then use VSCode to follow formatValue.

Expected Behavior

I would expect to end up in https://github.com/vega/vega-tooltip/blob/fee07c92ce6b1914650a0daf881ef403249e6374/src/formatValue.ts#L11

Actual Behavior

VSCode opens up the .d.ts file.

I used to compile declarations with tsc and declaration maps worked fine.

@wessberg
Copy link
Owner

wessberg commented Sep 9, 2020

Hey there. As you may know, rollup-plugin-ts bundles declarations, and it does this as a TypeScript Custom Transformer that visits SourceFiles through multiple steps (the major ones being bundling/merging, deconflicting, and tree-shaking).

TypeScript itself then takes care of diffing the source and output ASTs for each SourceFile and applies transformations to the declaration maps that will be emitted.

It is not that declaration maps doesn't work generally - they do, and we have several tests for them - but unfortunately it is true that they can break quite easily as soon as symbols from various SourceFiles are merged together in one

The primary reason behind it is that for TypeScript to be able to properly trace, say, a Declaration (so it can apply transformations to the declaration maps), it must be able to see a connection between the Declaration that ends up in the merged declaration file all the way back to the place and position where it was originally declared. And, when merging/bundling Nodes from other files, we can't just reference them, we have to clone them (which is why we use @wessberg/ts-clone-node for that. More info here), so this is where that connection is broken, and TypeScript loses track of that Node.

I would love to find a way to work around this, and intuitively I think a solution will require manually generating declaration maps and ignoring those generated by TypeScript, but that won't do since other Custom Transformers may be provided that applies further transformations to the bundled declarations, and these changes too should carry over to the declaration maps. I'll keep this issue open for future reference. And I'm open to suggestions as to how this might be resolved.

For the time being, I recommend disabling declaration maps. Based on what I can see, declaration maps are generally not supported by other declaration bundlers either.

@wessberg wessberg changed the title Declaration maps do not work Declaration maps do not always work Sep 9, 2020
@wessberg wessberg added the Bug Something isn't working label Sep 9, 2020
@domoritz
Copy link
Contributor Author

domoritz commented Sep 9, 2020

Thanks for the explanation and I definitely appreciate the complexity of the problem.

I wonder whether it would make sense to provide an option to generate .d.ts and .d.ts.map files for all source files rather than bundling them? What's the benefit of bundling in development?

@wessberg
Copy link
Owner

I've been thinking about that too, yes. I'm open to the idea of providing an option to opt out of bundling and just fall back to TypeScripts normal declaration mechanism.
There are some major gotchas with that approach though!

This plugin not only bundles declarations, but also fully supports code splitting of the declarations and more generally will respect the output structure and filenames produced by Rollup.

TypeScripts normal declaration system expects the source structure to be equivalent to the output structure, and so it is quite easy to end up in a situation where the definitions are placed in a different file than the actual value and then you have the problem of TypeScript suggesting importing from the wrong files and so on. For example, you may instruct Rollup to produce an output structure based on two entry chunks: a (pointing to src/foo.ts) and b (pointing to src/bar.ts). Rollup will then generate two bundles named a and b and split their common dependencies up into shared chunks. Now TypeScript, on the other hand, won't know anything about neither files called a and b nor any shared chunks, so it will complain when clients of that library attempts to import from either a or b.

So ideally, if we were to allow falling back to the built-in declarations system, we would have to do some work to at the very least rewrite import- and export specifiers to align at least the entry chunks with Rollup.

@domoritz
Copy link
Contributor Author

domoritz commented Sep 10, 2020

Again, I appreciate the complexity here. Thank you for the deep dive into the details.

I personally don't need chunks for the library projects I am working on so I will stick with rollup-typescript2 for now and switch over to rollup-plugin-ts later.

agilgur5 added a commit to agilgur5/ts-library-base that referenced this issue Apr 12, 2022
- use Babel to transpile TS with Jest for now
  - using https://jestjs.io/docs/getting-started#using-typescript-via-babel
  - plus some optimizations on caching with babel.config.js
    - also tried using babel.config.mjs, but I believe babel-jest
      doesn't support loading ES modules async yet
      - got "Error while loading config - You appear to be using a native ECMAScript module configuration file, which is only supported when running Babel asynchronously."
      - `@babel/register` recently added experimental support for this
        in preparation to the ESM-only release of Babel 8, so maybe
        related to that?

  - deps: add @babel/core, @babel/preset-env, and @babel/preset-typescript
    - configure preset-env to use `node: current` for test env only
      - will probably add a `.browserslistrc` for the rest

  - might change to `ts-jest` later, I'm not sure
    - may depend on how I decide to build (i.e. if I reuse Babel there)
      - currently pros/cons with each method
        - rpts2 has various issues I've experienced with TSDX and some
          that I've personally contributed fixes or features for
          - relatively easy to contribute to as it basically tries to
            use TS directly, but also lacks tests etc etc
        - @rollup/plugin-typescript I haven't used, but rpts2 and
          @wessberg/rollup-plugin-ts evolved out of its issues and are
          more popular than it
          - it has gotten more fixes etc over time, but not sure
          - also @rollup/plugins repo has lots of issues with difficult
            to understand changelogs that I've contributed improvements
            to before :/ (not that rpts2 is much better, but at least I
            know a good bit of the codebase)
        - @wessberg/rollup-plugin-ts suffers from being too good
          - like too many features that make it really complex to
            contribute to (in its current form at least) and lack of
            opt-out along with bugs in some of them
            - e.g. declaration maps + no opt-out of bundling: wessberg/rollup-plugin-ts#117 (comment)
          - it's really good at conforming to Rollup best practices and
            optimizing everything with chunks, supporting multi-entry,
            etc though
        - Rollup + Babel for transpile and `tsc` for declarations may
          run into similar declaration issues as the above issue when it
          comes to chunks
          - but is perhaps the simplest / most straightforward route
    - kind of off-topic to testing, but yes I did look into all this
      before just choosing Babel for testing, so figured I'd write it
      down somewhere
@NullVoxPopuli
Copy link

I noticed that declaration maps are no longer generated after typescript 4.2.
Not sure if related to this plugin or typescript itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants