(fix): declarationMaps (*.d.ts.map) should have correct sources
#488
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Feb 4, 2020
This was referenced Feb 7, 2020
agilgur5
force-pushed
the
dtsmap-fix
branch
2 times, most recently
from
f84cb0e to
846d984
Compare
|
Ok, so the breaking changes were split out and set to deprecated with a warning in #504 . Rebased this and removed the removals so now it is almost ready. #488 needs to be merged first and that now has a conflict from #476 and the |
3 tasks
- when declarationDir is set, `sources` gets set correctly, but otherwise its relative path will be incorrect - so always set declarationDir to paths.appDist
|
Ok, so I merged #468 (as I'm now a collaborator) and have rebased this PR with master to resolve conflicts. But now I'm not sure if this makes sense because it's more an upstream issue in ezolenko/rollup-plugin-typescript2#204 . That being said, this does workaround that issue. It just potentially introduces other issues with other Rollup plugins (e.g. #80) and with customizations like changing the output directory in |
agilgur5
added
the
topic: rollup-plugin-typescript2
agilgur5
added a commit
to agilgur5/react-signature-canvas
that referenced
this pull request
Apr 20, 2022
- 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
added a commit
to agilgur5/react-signature-canvas
that referenced
this pull request
Apr 20, 2022
- 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
added a commit
to agilgur5/ts-library-base
that referenced
this pull request
Jun 14, 2022
Basically this combines agilgur5/react-signature-canvas@b14060c and agilgur5/react-signature-canvas@5771d33 - and is based on my upstream fix: ezolenko/rollup-plugin-typescript2@ccd6815 - adds source maps for declarations for consumers - apparently I left this config out of this repo? - 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 - Previously, I ran into a bug in rpt2 when using it as a `configPlugin` and enabling `declarationMap`s - that bug was actually due to some code _I_ wrote in rpt2 and an edge case I didn't know existed (`configPlugin` usage means Rollup has no `output`) - so I eventually fixed it myself too - and in the past month I stepped up to help maintain rpt2 and fixed a ton of issues and improved lots of things per the release notes :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sourcesgets set correctly, butotherwise its relative path will be incorrect
Fixes #479 . Fixes #135
This is built on top of #468 , so please merge that first.
BREAKING:rootDir needed to be changed to ./src because the previous ./ causedtype declarations to be generated in dist/src/ instead of just dist/
the moveTypes function handled moving the declarations back intodist/, but .d.ts.map files would be relative to their original
output directory, and hence wrong
with this change, moveTypes is no longer needed and .d.ts.mapfiles will have the correct relative path, but the rootDir change
is breaking
I'm currently setting this as a Draft PR to discuss options for the breaking change. The templates and hence a lot of code out there hasrootDir: './'which is problematic/incorrect/buggy. But requiring users to change torootDir: './src'would be breaking.Two options around this in my head:1. Leave all the~#479 won't actually be solved for most users 😕 ~moveTypescode in for now and mark it as deprecated. This would mean types will be put intodist/if you have the oldrootDir: './'or the newrootDir: './src'(in the new case it's basically a no-op). But if using the old one, the types indist/are going to have incorrect relative paths in their declaration maps if using that feature -- this means that2. Set atsconfigOverrideforrootDir: './src'. But now users can't set their ownrootDir😕To be fair, I'm not entirely sure why this is included in the templates, because I don't believe TSDX fully supports custom
rootDir(pretty sure some custom paths will have buggy output).Both of these seem suboptimal to me~😕 ~EDIT: The deprecation was moved to #504 , where a warning was added, so it takes the first approach but with the warning prompting users to actually change their
rootDir.