Script to Prune .d.ts files (with tests) #4091
Conversation
schmidt-sebastian
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
|
schmidt-sebastian
force-pushed
the
mrschmidt/standalonescript
branch
from
d9ca6a4 to
7f6a4ac
Compare
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/standalonescript
branch
from
7f6a4ac to
73f2c9a
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/standalonescript
branch
from
73f2c9a to
1e359b5
Compare
scripts/prune-dts/prune-dts.ts
Outdated
| } else { | ||
| // Hide the type we are inheriting from and merge its declarations | ||
| // into the current class. | ||
| const privateType = typeChecker.getTypeAtLocation(type); |
There was a problem hiding this comment.
Do you need to handle extends, and implements differently? For example, implements can be just safely removed without merging.
There was a problem hiding this comment.
I considered this, but for this to work properly I would have to extract an extra symbol and then parse the declarations to determine whether whether the referenced type is a class or an interface. Not too bad... but it complicates this logic and doesn't actually change the result. I have added a TODO and a test that verifies the current behavior.
scripts/prune-dts/prune-dts.ts
Outdated
| if (ts.isIdentifier(type.expression)) { | ||
| const subclassName = type.expression.escapedText; | ||
| if (subclassName === localSymbolName) { | ||
| return symbol; |
There was a problem hiding this comment.
Just want to note that this is not always safe to replace a class with an inheriting class. Copied my comment from the previous PR:
In case of multiple public classes extending
PrivateFoo, we use the first one which seems arbitrary and is probably wrong - consideringPublicFoo1andPublicFoo2, both of which extendsPrivateFoo.doFooshould accept either of them, but after this change, it will only acceptPublicFoo1.
Our API typings probably don't have the case I described, so the code works, but we should be aware that this transformation can't be generalized to work for any d.ts files.
There was a problem hiding this comment.
I think we probably need to return a Union type in this case. I added a TODO, and I can fix this in a follow-up PR. I do think that this scripts needs a few iterations before it works for all d.ts file.
repo-scripts/prune-dts/package.json
Outdated
| @@ -0,0 +1,39 @@ | |||
| { | |||
| "name": "firebase-repo-scripts-prune-dts", | |||
| "version": "2.0.2", | |||
It's HACKWEEK... so it's time to pick up #3790 again. This time it is a standalone script with tests.
Almost all of the code in this PR are test cases (under /test) with BEFORE and AFTER files. The bulk of the change lines in the firestore example. The best way to review this PR might be to only look at the non d.ts files.