sources paths are incorrect in declarationMap files (*.d.ts.map) emitted by tsdx

[justingrant]

Current Behavior

When using the declarationMap option in tsconfig.json, the sources array in declaration map files does not contain correct relative paths. Repro steps:

1. git clone https://github.com/justingrant/ts-declaration-map-repro.git
2. cd ts-declaration-map-repro
3. yarn
4. yarn run with-tsc - this will simply run tsc against this repo.
5. Open ./dist/index.d.ts.map. It will begin with:
   {"version":3,"file":"index.d.ts","sourceRoot":"","sources":["../src/index.tsx"]. Note the correct relative path to index.tsx
6. yarn run with-tsdx which uses tdsx instead of tsc but uses the same TS config.
7. Open ./dist/index.d.ts.map again. Here's what you'll see:
   {"version":3,"file":"index.d.ts","sourceRoot":"","sources":["src/index.tsx"]. Note that the path to index.tsx is incorrectly relative to the project root, not relative to dist.

Expected behavior

Correct relative path to index.tsx (../src/index.tsx) in the map file's sources array.

Suggested solution(s)

I'm assuming that there's a way to configure rollup-plugin-typescript2 so that declaration maps are emitted with the correct paths.

Additional context

Note that only declaration map files are affected. Relative paths in regular sourcemap files works fine.

I'm not sure if this is a problem with tsdx or rollup-plugin-typescript2. But I didn't see any mention of this problem in rollup-plugin-typescript2's issue list, so I figured that tsdx may be the culprit because it's the new kid on the block. I'm assuming (perhaps naively) that if the problem was in rollup-plugin-typescript2 that it would have been reported in its GitHub issues already.

At first I thought that this problem was related to #465, but I think that issue is solely about how to handle the declarationDir config setting which I'm not using at all. Instead, I'm relying on the same outDir folder for both transpiled output and for declaration output.

This issue may be a dupe of #135, but I'm not sure, because #135 has a much more complex config. But the problem does look similar: in both issues the relative path to the source files is missing in declaration maps emitted by tsdx.

BTW, great library! Glad you're building this.

Your environment

Software	Version(s)
TSDX	[email protected]
TypeScript	[email protected]
Browser	n/a (this is a build issue)
npm/Yarn	[email protected]
Node	v12.13.1
Operating System	MacOS Catalina 10.15.2

[agilgur5]

Since I reviewed #465 / #468, this definitely does seem related to them, as well as #135 . Based on the comments in #135, it sounds like adding declarationDir might work as a workaround for this once #468 is merged.

A potentially relevant note is that there is this function in the codebase moveTypes:

tsdx/src/index.ts

Lines 104 to 112 in 4f6de10

	async function moveTypes() {
	try {
	// Move the typescript types to the base of the ./dist folder
	await fs.copy(paths.appDist + '/src', paths.appDist, {
	overwrite: true,
	});
	await fs.remove(paths.appDist + '/src');
	} catch (e) {}
	}

Type definitions originally get generated by rpts2 in dist/src/ and are then moved to dist/ by TSDX.

I suspect that if TSDX uses, internally as a default, declarationDir: 'dist/', that might fix the issue, or at least get closer.

As they're generated in dist/src/ instead of the output directory dist/ it might also be a bug in rpts2, not really sure at this time.

This comment is also still relevant, that src/ isn't always published with libraries, only type defs. But your issue suggests that the paths are incorrect even if src/ was published.

[agilgur5]

Potentially related to ezolenko/rollup-plugin-typescript2#136 , which goes over why rpts2's type definitions output structure mirrors that of your source structure

[agilgur5]

Looked into it a bit more and there's very little code in rpts2 around .d.ts.map files, they're entirely produced by the TS Language Service. So if anything, the problem is how the input is passed in.
Hard for me to really dig into it any further as that gets into Language Service specifics which I don't know anything about. Can try replicating this with plain rollup and rpts2 and see if it still occurs and then can see if we can get any help from the rpts2 folks.

---

Something else potentially related is that, in your case, you have specified an outDir in tsconfig.json, which tsc can interpret, whereas tsdx does not currently interpret it (there's a number of issues around this). rpts2 should still read it (afaik), but TSDX code will break the output due to multiple output formats etc

[justingrant]

@agilgur5 - To verify your notes above, I hacked useTsconfigDeclarationDir: true in createRollupConfig.js's rpts2 and the declaration map's sources paths are emitted properly. This is good news, because this issue should be fixed by #468.

That said, setting useTsconfigDeclarationDir: true also works if I don't have a declarationDir setting in my tsconfig.json (tsc handles that case by using outDir for the declaration directory). Unfortunately, the current code in #468 won't work for that case. Excerpt:

tsdx/src/createRollupConfig.ts

Lines 156 to 158 in 77bc303

	useTsconfigDeclarationDir: Boolean(
	tsconfigJSON?.compilerOptions?.declarationDir
	),

If my tsconfig has no declarationDir, then the code above returns false, which is not desired. I'll comment on #468 to continue the conversation over there.

BTW, while stepping through tsdx code to investigate this problem, I discovered two additional problems with tsconfig parsing: #483 (comments and other extended JSON features are not handled the same as tsc does) and #484 (extends in tsconfig files is ignored by tsdx but handled properly by tsc). Both of these have a common root cause: tsdx is parsing tsconfig.json differently than tsc. Luckily TS exposes its tsconfig parser in an API.

I just updated the repro repo for this issue (https://github.com/justingrant/ts-declaration-map-repro) to remove problematic JSON that triggered #483 these issues, to avoid complicating reproes of this issue. I also removed declarationDir from my repo's config to better demonstrate the problem noted above with #468.

[agilgur5]

@agilgur5 - To verify your notes above, I hacked useTsconfigDeclarationDir: true in createRollupConfig.js's rpts2 and the declaration map's sources paths are emitted properly. This is good news, because this issue should be fixed by #468.

Thanks for checking this! So #468 will create a workaround for this bug. I think we can try adding declarationDir: paths.appDist to tsconfigDefaults as a full fix for this as I wrote above. I'll create a PR for that on top of #468's code.

That said, setting useTsconfigDeclarationDir: true also works if I don't have a declarationDir setting in my tsconfig.json (tsc handles that case by using outDir for the declaration directory). Unfortunately, the current code in #468 won't work for that case.
If my tsconfig has no declarationDir, then the code above returns false, which is not desired. I'll comment on #468 to continue the conversation over there.

Per my comments there, let's not mix outDir support with declarationDir support. As I wrote here, outDir is not really supported by TSDX right now, so adding useTsconfigDeclarationDir support for it will still cause other issues.

The above declarationDir: paths.appDist is probably the optimal fix, and when outDir support is added we'll have to override that.

BTW, while stepping through tsdx code to investigate this problem, I discovered two additional problems with tsconfig parsing: #483 (comments and other extended JSON features are not handled the same as tsc does) and #484 (extends in tsconfig files is ignored by tsdx but handled properly by tsc). Both of these have a common root cause: tsdx is parsing tsconfig.json differently than tsc. Luckily TS exposes its tsconfig parser in an API.

Great finds! Will follow-up in those.

I also removed declarationDir from my repo's config to better demonstrate the problem noted above with #468.

Should probably make this a separate branch as it's a more specific problem.

[justingrant]

let's not mix outDir support with declarationDir support

Agreed. My outDir is ./dist. If that's tsdx's default then that explains why I didn't notice any problems with outDir. ;-)

I think we can try adding declarationDir: paths.appDist to tsconfigDefaults as a full fix for this as I wrote above. I'll create a PR for that on top of #468's code.

That sounds like a good solution.

Should probably make this a separate branch as it's a more specific problem.

The current master branch of https://github.com/justingrant/ts-declaration-map-repro has no declarationDir and no trailing commas. The original version had "declarationDir": "./dist" and one trailing comma. My goals when revising the repo were:

• remove tsconfig syntax (a trailing comma) that triggered tsdx silently fails parsing of tsconfig files containing comments, trailing commas, etc. that TSC accepts #483, because that would have prevented verifying Enable useTsconfigDeclarationDir if declarationDir #468 because tsconfig.json was not parsed successfully. I didn't think that my repo needed a repro for tsdx silently fails parsing of tsconfig files containing comments, trailing commas, etc. that TSC accepts #483 because that repro was so obvious: just add a trailing comma or a comment to tsconfig.
• clarify the case which is not going to be fixed by Enable useTsconfigDeclarationDir if declarationDir #468, which is the case when declarationDir is not included in tsconfig. AFAIK, the other case (where declarationDir is present) is already covered by unit tests included with Enable useTsconfigDeclarationDir if declarationDir #468.

Given the explanation above, would a new branch still be helpful? I'm happy to add another branch but I'm not sure what should be in it. Could you clarify what other repro(s) you think would be useful in this repo?

[agilgur5]

Agreed. My outDir is ./dist. If that's tsdx's default then that explains why I didn't notice any problems with outDir. ;-)

Or rather, not yet you didn't 😅😅😅 See #468 (comment) for some of the other problems around outDir.

• remove tsconfig syntax (a trailing comma) that triggered tsdx silently fails parsing of tsconfig files containing comments, trailing commas, etc. that TSC accepts #483, because that would have prevented verifying Enable useTsconfigDeclarationDir if declarationDir #468 because tsconfig.json was not parsed successfully. I didn't think that my repo needed a repro for tsdx silently fails parsing of tsconfig files containing comments, trailing commas, etc. that TSC accepts #483 because that repro was so obvious: just add a trailing comma or a comment to tsconfig.

Yea that change is fine and I agree I don't think it necessarily needs a repro either. Though repros are always helpful so a maintainer doesn't need to, like, create the code themselves. This case is also a bit unique as it's a silent error, so you'd only notice when things are different from expected.

• clarify the case which is not going to be fixed by Enable useTsconfigDeclarationDir if declarationDir #468, which is the case when declarationDir is not included in tsconfig. AFAIK, the other case (where declarationDir is present) is already covered by unit tests included with Enable useTsconfigDeclarationDir if declarationDir #468.

Ok, gotcha. I think the original case is easier to understand the root cause, but that makes sense.
Well, the unit tests don't check that index.d.ts.map is correct -- just that it exists -- which is the core issue here.

Given the explanation above, would a new branch still be helpful? I'm happy to add another branch but I'm not sure what should be in it. Could you clarify what other repro(s) you think would be useful in this repo?

I think this is fine as at least I've already grasped it and will probably be the one to PR a fix. Thanks for the clarification around the changes!

[justingrant]

@agilgur5 - All sounds good. I had one question about your proposed fix: (from your comment in #468)

The proposed fix I reference above would add a default declarationDir, so that would ensure the same code path regardless.

Once that fix is in place, will useTsconfigDeclarationDir always be set to true, instead of the conditional code in #468? If not, then how would it fix the current issue in the case where tsconfig.json doesn't have a declarationDir?

[agilgur5]

Once that fix is in place, will useTsconfigDeclarationDir always be set to true, instead of the conditional code in #468?

Yup, it would have to be. I'm actually working on that right now and stuck on some failing tests here.

Running into an issue where even with declarationDir set to dist/(rather paths.appDist), typings still get put into dist/src/. The index.d.ts.map file is correct from that directory (../../src/index.ts), but still confused why it's not in dist/.
Weirdly enough, when using declarationDir: 'typings/' as in #468 , index.d.ts.map is in typings/, no added src/ and is also correct (../src/index.ts) 😕

[agilgur5]

Figured out that the problem was that #468 changed the rootDir to ./src from ./, changing all tests to also use ./src fixes this... but all templates and lots of code out there from the templates have rootDir: './' already set, so requiring rootDir: './src' is actually breaking 😭 😞 😖

Can do some internal deprecation of code to workaround this, but super not ideal 😕

[agilgur5]

Just noticed your repro also has rootDir: './src' set. When it's ./, we get "sources": ["index.ts"], which would point to the compiled version. That's still not correct, and without moveTypes it'd be in dist/src/ still pointing to index.ts (which is in plain dist), but worth noting.

[agilgur5]

@ibraelillo this issue is already tagged as "workaround available" because there is already one available as is written here and in the related issues, it is setting your declarationDir to dist. Which is quite a bit simpler than yours.

I also already added a PR to do that by default in #488 that is linked above but have not merged it because it is actually an upstream issue and the change is a bit breaking if anyone is using a Rollup plugin that operates on declarations with TSDX (c.f. #80 ), but otherwise safe to use as a workaround.

And I had already filed the upstream issue ezolenko/rollup-plugin-typescript2#204 linked above and then made an upstream PR myself linked above ezolenko/rollup-plugin-typescript2#221 . I've been quite busy at work so unfortunately I haven't been able to put in much OSS time, but that was actually my top priority to get into for v0.13.x (now I've waited too long to release v0.14 as a result). The maintainer did not provide much support there either so some bugs have caused it to stall.

[ibraelillo]

@agilgur5 I've done this workaround because of the fix you proposed was not working for me. I'm working with several libraries created with tsdx on a large monorepo and they all present the same issue, actually fixed for me at least with this workaround. I've passed two days browsing and trying any kind of fixes and only that one worked well on every library we have in the monorepo without any further modification.

I'm still pending results on this topic, if any other solution comes to light.

[agilgur5]

@ibraelillo well I'm not sure why it doesn't work for you, #488 has tests included and they pass. If something doesn't work, would need a minimal reproduction to ascertain anything. Though to be clear, the real fix and real issue is upstream and not here and that's where attention should be focused.

[paynecodes]

There's a winding web of issues here that are difficult to follow while attempting to ascertain who/how/if there is a fix for this. Shouldn't this be fixed by default without needing to use (or add) declarationDir? To me, declarationMap files are a major dx improvement that a tool like this would make certain were working out of the box.

[agilgur5]

@paynecodes my comment above summarizes it, but here's a longer form version of that:

Shouldn't this be fixed by default without needing to use (or add) declarationDir?

1. Using declarationDir is a workaround that so happens to solve the issue. This includes setting it to dist/.
   • I added the dist/ use by default in (fix): declarationMaps (*.d.ts.map) should have correct sources #488, however aside from being not a root cause fix, that option is actually breaking.
   • The useTsconfigDeclarationDir of rpts2 makes TS output the declarations instead of Rollup, which is why it fixes it. That can break downstream Rollup config via tsdx.config.js if anyone were relying on that to make further declaration transformations (e.g. .d.ts bundles #80)

To me, declarationMap files are a major dx improvement that a tool like this would make certain were working out of the box.

2. This is a bug upstream, not here. I filed declaration maps (*.d.ts.map) have incorrect relative paths in sources unless declarationDir is set ezolenko/rollup-plugin-typescript2#204

   • The maintainer helped find the location of the bug, however changing that to be correct was infeasible due to how Rollup's plugin lifecycle worked
   • The repo/library itself is only partially maintained I would say; lot of unresolved or unresponded to issues that have been building up (Type-only/Emit-less TS files aren't being checked #725, Support for "composite" TS projects / monorepos? #828 are among the current issues duplicated here in TSDX). There's only two alternatives and they have their differences and bugs too (TSDX migrated to @wessberg/rollup-plugin-ts for v0.10.0 before switching back due to a variety of issues, though some seem to have fixes out), unfortunately.

3. I eventually filed a PR to fix it upstream as well: (fix): declaration maps should have correct sources ezolenko/rollup-plugin-typescript2#221

   • This accounts for the lifecycle issue above, and I had to dive fairly deep into the codebase (which is not the simplest) in order to understand this, as well as had to understand the Rollup plugin lifecycle.
   • That stalled and I was given a not very helpful response from the maintainer saying it broke something without any further guidance
   • I looked into it periodically March - June-ish and tested a lot of things to no avail. This was originally slated to be fixed for v0.13.x as the milestone I added here says.
     • That said, I had to explore more of the codebase so I'm more familiar with it now (including exploring Type-only/Emit-less TS files aren't being checked #725 ), so I have an idea of how to fix it (and potentially deprecate and make useTsconfigDeclarationDir no longer necessary), but I need time to work that out and test it.

4. Neither this nor Can (should?) tsdx create declaration maps by default, like we do with sourcemaps? #490 have any up-votes (there's only 1 and it's me), and most people don't know what declarationMaps even are, so the need / complexity ratio is pretty low in terms of prioritizing this.

[paynecodes]

@agilgur5 Thank you for your insightful reponse! If only more people know about declarationMap, I don't think think they would want to work without it. At least not in a monorepo context ;)

[agilgur5]

Just updated my PR upstream in ezolenko/rollup-plugin-typescript2#221, think I've solved the problem now. Summarized a lot of the work and debugging I did in the comments there (ezolenko/rollup-plugin-typescript2#221 (comment))