(fix): declaration maps should have correct sources #221

agilgur5 · 2020-03-29T06:19:31Z

change declarationDir default to be Rollup's output destination
instead of cwd
- previously, it was set to cwd if useTsconfigDeclarationDir wasn't
  true, however, declarations aren't output to cwd, but to Rollup's
  output destination, so this was incorrect
also change the one line in the docs that says it'll be
process.cwd(); every other reference says it'll be the output dest

Fixes #204 and downstream jaredpalmer/tsdx#479 . Better solution to downstream jaredpalmer/tsdx#488

Couldn't change get-options-overrides because the options hook only gets InputOptions; it's not till generateBundle that we get access to OutputOptions.

RE: multiple output dirs: Rollup will call generateBundle for each output as far as I understand. InputOptions is passed to rollup which generates a bundle object, and bundle.write is called with each OutputOptions -- it's a two step+ process. I think that's also why the type is OutputOptions and not OutputOptions[] (though the code doesn't use Rollup's Plugin interface; but that also defines it as generateBundle with just OutputOptions).

Would be great to add tests to ensure the declaration maps are correct now, but there doesn't seem to be any test set-up 😕 #135 seems to partially address that but was never completed.
Can see jaredpalmer/tsdx#488 for how I tested sources there

ezolenko · 2020-04-08T19:39:07Z

Sorry for ignoring this for a while -- this breaks something -- if you build the plugin itself with this change, it drops types into dist\node_modules\.cache\rollup-plugin-typescript2\placeholder instead of dist like rollup config says.

To test do npm build (check changes in git) and then do npm build-self (check git again and you'll see lots of files moved).

agilgur5 · 2020-09-30T02:24:17Z

So I had to learn a lot about Rollup plugins and lifecycle to write this PR. Then without further guidance here had to do a lot of learning of this codebase and the TS API. As a result, it took me a while to understand how to fix this.

Debugging the PR

it drops types into dist\node_modules\.cache\rollup-plugin-typescript2\placeholder instead of dist like rollup config says.

I figured out why this was happening relatively quickly, that's because outDir is set to ${cacheRoot}/placeholder, and by default declarationDir is equivalent to outDir. That's on this line:

rollup-plugin-typescript2/src/get-options-overrides.ts

Line 17 in 2f51110

    
           outDir: `${cacheRoot}/placeholder`, // need an outdir that is different from source or tsconfig parsing trips up. https://github.com/Microsoft/TypeScript/issues/24715

I added some logging during output that displays this as well. While sources is correct now, the entry was output to the wrong place, ${cacheRoot}/placeholder:

entry name:  /Users/agilgur5/Desktop/GitHub/rollup-plugin-typescript2/node_modules/.cache/rollup-plugin-typescript2/placeholder/partial.d.ts.map
dtsmap entry text:  {"version":3,"file":"partial.d.ts","sourceRoot":"","sources":["../../../../src/partial.ts"],"names":[],"mappings":"AAAA,MAAM,CAAC,OAAO,MAAM,OAAO,CAAC,CAAC,IAAI;KAAG,CAAC,IAAI,MAAM,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,CAAC,CAAC;CAAG,CAAC"}

That can't be changed to Rollup's output.dir for the same reason declarationDir can't be; per my opening comment "it's not till generateBundle that we get access to OutputOptions."

Current PR doesn't work

In the current PR, the changes apparently have no effect on the output because they change declarationDir during generateBundle, but the declarations were already created during transform:

rollup-plugin-typescript2/src/index.ts

Line 261 in 2f51110

declarations[key] = { type: result.dts, map: result.dtsmap };

Trying different solutions

Between March - June-ish, I came back to this a few times to add more logging and understand this as well as to try to somehow set declarationDir properly, but failed a lot because this isn't possible basically.

The LanguageServer, which has the tsconfig declarationDir setting, is created during options:

rollup-plugin-typescript2/src/index.ts

Line 114 in 2f51110

    
           servicesHost = new LanguageServiceHost(parsedConfig, pluginOptions.transformers, pluginOptions.cwd);

I thought to lazily initialize this, but that's not possible, it's needed in the next plugin lifecycle steps, especially transform.

It can't be done in generateBundle where I was trying in the current PR -- it would have to be done in or before transform -- but OutputOptions like output.dir aren't passed until generateBundle. That causes quite a dilemma.

But Rollup is designed that way intentionally, any output transformations should be left until generateBundle.

Solution

Basically the only option is to transform the declaration maps' sources to the correct relative path to output.dir during generateBundle. I didn't want to resort to doing so as it's one more transform and seemingly in the wrong lifecycle step, but that seems to be how it's designed. It's also a bit expensive performance-wise as each declaration map JSON has to be parsed and then stringified, and sometimes the full source (in this case declaration source) is inside those, making that more perf-heavy.

I checked @wessberg/rollup-plugin-ts to see if it was handled similarly there, and it indeed seems to be (it also bundles declarations, so the logic is a bit different, but still relevant):

map = JSON.parse(text) as SourceMap;
map.file = options.declarationPaths.fileName;

So I'm going to rework this PR to have a similar approach and transform sources during generateBundle. Should hopefully have that done soon.

Other issues

But TSDX is having a lot of other issues with rollup-plugin-typescript2, such as bugs with emitless/type-only imports (jaredpalmer/tsdx#725) and composite projects (jaredpalmer/tsdx#828), among others (jaredpalmer/tsdx#796)

src/get-options-overrides.ts

agilgur5 · 2020-09-30T17:58:32Z

src/index.ts

+					// don't mutate the entry because generateBundle gets called multiple times
+					let entryText = entry.text


this took me a few hours to debug and was by far the most time-consuming piece 😕 I originally changed entry.text itself which then got changed on top of itself for each output...

Ideally I think generateBundle should have some caching so this isn't repeated per output. I could do that with a cache key inside of declarations{}, but there's a good question as to what that cache key should be (in this case, I think the declarationDir variable is the only thing that is subject to change, but that sounds brittle if more stuff gets added).

src/index.ts

- previously, declarationDir was set to cwd if useTsconfigDeclarationDir wasn't true, however, declarations aren't output to cwd, but to Rollup's output destination, so this was incorrect - instead, don't set declarationDir, which defaults it to outDir, which is currently set to a placeholder - previously, it rewrote declarations to output to Rollup's dest from cwd, now rewrite from outDir placeholder instead - and add a rewrite of sources to match relative path from Rollup's output dest instead of outDir placeholder - also change the one line in the docs that says it'll be `process.cwd()`; every other reference says it'll be the output dest

agilgur5 · 2020-09-30T23:57:43Z

@ezolenko this should be good to go now -- if you run npm run build && npm run build-self it will change the declaration maps' sources to be correct now, and the files should all still be in the right places.

As I wrote in my opening comment, a test set-up like #135 would be great to have; could add the test I have in jaredpalmer/tsdx#488 here then.

ezolenko · 2020-10-02T14:46:53Z

@agilgur5 looks good now. The only problem I see is sources in type maps are in windows format if I build it on windows: "..\\src\\context.ts". Any idea if this will confuse existing tooling?

nvm, fixed it

agilgur5 · 2020-10-02T15:34:34Z

Ah, I don't run on a Windows machine, but if that's not normal behavior (wouldn't know), then could use path.normalize

ezolenko · 2020-10-02T16:50:48Z

@agilgur5 Ok, PR is in master, could you doublecheck and I'll do a release

agilgur5 · 2020-10-02T20:44:07Z

Looks like you added normalize in front of the relative call so looks correct to me.

I saw you added normalize in front of the emitFile fileName mapping to however -- that didn't have normalize in a previous version, I only changed the path, not how it was mutated: ec0568b#diff-f41e9d04a45c83f3b6f6e630f10117feL368 . I don't know if it should be normalized, but that is different from what it was before.

On install + build + build-self, the package-lock.json auto-updates to the new version and the cjs.js.map and es.js.map get updated for some reason (not possible to read the diff to understand since it's a one-liner). Not sure what that means. Otherwise, don't see any weird anomalies.

agilgur5 · 2020-10-09T01:49:55Z

@ezolenko any update on the above or a release?

agilgur5 · 2020-10-15T18:52:01Z

@ezolenko you merged some of my other PRs, but didn't comment on the above and there still hasn't been a release 😕

ezolenko · 2020-10-16T19:50:37Z

Sorry, was distracted.

When you run npm install it will pull patch updated of deps and I that might affect source maps a bit (maybe line number changed because of comments or something).

I'll do a release later today.

agilgur5 · 2020-10-16T19:55:11Z

What about this part?

I saw you added normalize in front of the emitFile fileName mapping to however -- that didn't have normalize in a previous version, I only changed the path, not how it was mutated: ec0568b#diff-f41e9d04a45c83f3b6f6e630f10117feL368 . I don't know if it should be normalized, but that is different from what it was before.

ezolenko · 2020-10-16T20:01:29Z

Re normalize for emit file, seems to have no effects either way, rollup handles it fine, but might affect downstream plugins that want to modify maps. Normalize is more correct in principle though, so I guess we should keep it and see if problems show up.

agilgur5 · 2020-10-16T20:16:56Z

Ok, yea I read in some Rollup issues (can't find which one it was) that it allows whichever internally, which means plugins have to handle and do the normalization. rollup/rollup#2952 suggests this is particularly relevant for resolveId

- fancy source maps for declarations - apparently I left these out of `ts-library-base` so they didn't get copied here either - but most of my tsconfig there was copied too, so I suspect I left it out either because of @wessberg/rollup-plugin-ts's bugs with it or because TSDX previously had bugs with it - c.f. ezolenko/rollup-plugin-typescript2#221 and the worse downstream version I had written first: jaredpalmer/tsdx#488 - Unfortunately I did encounter a small error with declaration maps when using rpts2 as a configPlugin, due to an unexpected edge case in code I wrote myself in the above PR - wrote up an issue for it and will probably PR it myself too: ezolenko/rollup-plugin-typescript2#310 - as a workaround, I wrote a small `tsconfig.rollup.json` to be used with rpts2 as a configPlugin that disables `declarationMap` - and also adds some optimizations over my base tsconfig - took me a bit to figure out how to use options with configPlugins, as that was one of the reasons I didn't use @rollup/plugin-babel as the configPlugin - I still can't get it to read my `babel.config.js` even with options as it has no option for this (it auto-detects it) :/

agilgur5 mentioned this pull request Mar 29, 2020

(refactor): use Rollup's Plugin interface instead of manual types #222

Merged

ezolenko self-assigned this Mar 31, 2020

agilgur5 mentioned this pull request Jun 7, 2020

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

Open

agilgur5 mentioned this pull request Aug 22, 2020

RFC: Run prettier on dev bundle output jaredpalmer/tsdx#82

Open

agilgur5 force-pushed the fix-declaration-map-sources branch from f107eba to a325fca Compare September 30, 2020 05:57

agilgur5 commented Sep 30, 2020

View reviewed changes

agilgur5 force-pushed the fix-declaration-map-sources branch from a325fca to ec0568b Compare September 30, 2020 23:42

agilgur5 mentioned this pull request Oct 1, 2020

Insert bin prefix for script packages jaredpalmer/tsdx#338

Open

ezolenko merged commit ee33209 into ezolenko:master Oct 2, 2020

agilgur5 mentioned this pull request Oct 6, 2020

fix/deps: upgrade rollup-plugin-typescript2 to fix cache issue jaredpalmer/tsdx#896

Merged

agilgur5 mentioned this pull request Oct 11, 2020

Node.js 13.14.0 'No "exports" main resolved in' @babel/helper-compilation-targets/package.json jaredpalmer/tsdx#900

Closed

dependabot bot mentioned this pull request Mar 15, 2021

chore(deps-dev): bump rollup-plugin-typescript2 from 0.27.3 to 0.30.0 ali-ieu/ieu-js-sdk#17

Merged

agilgur5 mentioned this pull request Apr 20, 2022

Runtime error when using as configPlugin with "declarationMap": true #310

Closed

agilgur5 added the kind: bug Something isn't working properly label May 7, 2022

Repository owner locked as resolved and limited conversation to collaborators May 7, 2022

agilgur5 added the topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX label Jun 25, 2022

agilgur5 deleted the fix-declaration-map-sources branch July 2, 2023 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix): declaration maps should have correct sources #221

(fix): declaration maps should have correct sources #221

agilgur5 commented Mar 29, 2020 •

edited

Loading

ezolenko commented Apr 8, 2020

agilgur5 commented Sep 30, 2020 •

edited

Loading

agilgur5 Sep 30, 2020

agilgur5 commented Sep 30, 2020 •

edited

Loading

ezolenko commented Oct 2, 2020 •

edited

Loading

agilgur5 commented Oct 2, 2020

ezolenko commented Oct 2, 2020

agilgur5 commented Oct 2, 2020 •

edited

Loading

agilgur5 commented Oct 9, 2020

agilgur5 commented Oct 15, 2020

ezolenko commented Oct 16, 2020

agilgur5 commented Oct 16, 2020

ezolenko commented Oct 16, 2020

agilgur5 commented Oct 16, 2020

		// don't mutate the entry because generateBundle gets called multiple times
		let entryText = entry.text

(fix): declaration maps should have correct sources #221

(fix): declaration maps should have correct sources #221

Conversation

agilgur5 commented Mar 29, 2020 • edited Loading

ezolenko commented Apr 8, 2020

agilgur5 commented Sep 30, 2020 • edited Loading

Debugging the PR

Current PR doesn't work

Trying different solutions

Solution

Other issues

agilgur5 Sep 30, 2020

Choose a reason for hiding this comment

agilgur5 commented Sep 30, 2020 • edited Loading

ezolenko commented Oct 2, 2020 • edited Loading

agilgur5 commented Oct 2, 2020

ezolenko commented Oct 2, 2020

agilgur5 commented Oct 2, 2020 • edited Loading

agilgur5 commented Oct 9, 2020

agilgur5 commented Oct 15, 2020

ezolenko commented Oct 16, 2020

agilgur5 commented Oct 16, 2020

ezolenko commented Oct 16, 2020

agilgur5 commented Oct 16, 2020

agilgur5 commented Mar 29, 2020 •

edited

Loading

agilgur5 commented Sep 30, 2020 •

edited

Loading

agilgur5 commented Sep 30, 2020 •

edited

Loading

ezolenko commented Oct 2, 2020 •

edited

Loading

agilgur5 commented Oct 2, 2020 •

edited

Loading