fix(typescript): avoid resolving definition files with arbitrary extensions#1949
fix(typescript): avoid resolving definition files with arbitrary extensions#1949ychavoya wants to merge 3 commits intorollup:masterfrom
Conversation
| */ | ||
| export function isDeclarationOutputFile(name: string): boolean { | ||
| return /\.d\.[cm]?ts$/.test(name); | ||
| return /\.d(\..+)?\.[cm]?ts$/.test(name); |
There was a problem hiding this comment.
I was wondering whether an arbitrary string is allowed between .d. and .ts, or if only a restricted set of characters are allowed. It turns out any arbitrary string is allowed here:
tl;dr this regexp sounds correct to me.
packages/typescript/test/test.js
Outdated
| t.is(warnings.length, 1); | ||
| t.is(warnings[0].code, 'UNRESOLVED_IMPORT'); | ||
| t.is(warnings.length, 2); | ||
| t.is(warnings[0].pluginCode, 'TS5110'); |
There was a problem hiding this comment.
What is this warning? Is it about the module value not being aligned with the moduleResolution value?
Shouldn't we resolve it instead?
There was a problem hiding this comment.
Resolved! I was not sure if I should update this to include the warning, or this other statement https://github.com/rollup/plugins/pull/1949/changes#diff-9b47757e1347eae5ab5cefedce0669826e925f9b073a02815b9507b782f2211bL1540 since changing the module changes how the code is output in the test.
There was a problem hiding this comment.
What the test cares about is the unresolved import, so I'd say it's better if we avoid any other unnecessary noise :P
| } | ||
|
|
||
| const configCache = new Map() as typescript.ESMap<string, ExtendedConfigCacheEntry>; | ||
| const configCache = new Map() as Map<string, ExtendedConfigCacheEntry>; |
There was a problem hiding this comment.
Nit: could we avoid the cast here by using new Map<string, ExtendedConfigCacheEntry>() instead?
There was a problem hiding this comment.
Thanks, I missed that when changing it
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
Looks good apart from this minor warning-related comment!
|
These new changes look good! Can you squash them into the commits that introduced what they're fixing? |
ychavoya
force-pushed
the
arbitrary-extensions
branch
from
c69a081 to
54649a3
Compare
|
Done, I squashed the changes into the typescript upgrade, so now there are only the 3 commits I initially added. Let me know if you want them combined as well |
|
@shellscape, would you mind having a look? |
2 participants
Rollup Plugin Name:
typescriptThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: resolves #1858
Description
This PR updates the regular expression used to detect declaration files (
*.d.ts) to include declaration files for arbitrary extensions, such as the ones allowed with theallowArbitraryExtensionscompiler option (*.d.*.ts). By including these in the regex, they are not resolved and therefore prevent the error displayed in #1858.Also, in order to add a test for this behavior, the typescript version in the dev dependencies was upgraded to v5 and therefore some other pieces of code had to be changed.