-
Notifications
You must be signed in to change notification settings - Fork 215
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
getDTName logic is brittle #1125
Comments
I'm pretty sure the problem is just that since dts-gen wasn't previously a part of this repo, it didn't get to use any of the other |
Thanks, I wasn't aware of that - but these functions are brittle in an identical way. |
This same brittleness would apply to TS too: https://github.com/microsoft/TypeScript/blob/c0b3ff2da1a5c1480710a81a994659ea706af484/src/compiler/moduleNameResolver.ts#L3221 But to be clear, we're talking about the condition under which there exists an npm package that includes I agree it's brittle, but it's never come up, and every version of TS would have this bug, so... |
I completely agree, and it's quite certainly got a likelihood of causing problems somewhere between 0 and slightly above that :-) However, I wanted to file the issue anyways, so the flaw is documented, and if you think the double-separator approach would be "fine" (esp since nobody would likely exercise it) I'd be happy to make a PR both here and in TS directly. |
The current logic in https://github.com/microsoft/DefinitelyTyped-tools/blob/HEAD/packages/dts-gen/src/names.ts#L1 is basically "if it's a scoped package, replace the slash with
__
".Unfortunately, this fails in the event of an existing package with a double underscore. I don't have a concrete existing example, so it's not an urgent problem :-) but, eg the types for
@eslint/js
are in@types/eslint__js
, but anyone could make a packageeslint__js
, and then what would the types package for that be?Obviously changing existing DT package names would be very disruptive, but if we could come up with, for non-scoped packages that have a double underscore, a new convention, then that'd close this theoretical hole (even if nobody ever needed it).
One idea is
eslint__js
→eslint____js
, eg replace__
with____
. this would mean thateslint____js
(4 underscores) would become 8, etc, but the likelihood of this running into package name limits is even lower than the likelihood of 2 underscores being used.Essentially, any replacement scheme would make
__
in a regular package name be transformed into something else, and that would require a similar cascade as above.The text was updated successfully, but these errors were encountered: