Export internal functions needed by dtsBundler.mjs #52941
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
@jakebailey I'm going through old PRs: Is this still worth bringing up at a design meeting? |
|
Yes; it's still an API that I use unsafely in dtsBundler and would like to make public. |
|
Undrafting since we seemed to have added a new API for similar reasons. I think I also should propose |
jakebailey
changed the title
|
Updated for further discussion; I'm not sure if we should make TypeScript/src/services/textChanges.ts Lines 1370 to 1376 in 85c2577 |
|
Would definitely like @rbuckton to give his opinion on the visitor change here; we were unsure in the meeting which way was good but we thought that making the context option sounded fine, so I updated the PR to do so. |
src/compiler/visitorPublic.ts
Outdated
| @@ -580,20 +581,20 @@ export function visitCommaListElements(elements: NodeArray<Expression>, visitor: | |||
| * @param visitor The callback used to visit each child. | |||
| * @param context A lexical environment context for the visitor. | |||
| */ | |||
| export function visitEachChild<T extends Node>(node: T, visitor: Visitor, context: TransformationContext): T; | |||
| export function visitEachChild<T extends Node>(node: T, visitor: Visitor, context?: TransformationContext): T; | |||
There was a problem hiding this comment.
I would rather these be | undefined instead of optional, as it's too easy to neglect to supply the context when it is needed. We do the same for NodeFactory.createTempVariable, where you have to explicitly pass undefined for the recordTempVariable callback parameter.
There was a problem hiding this comment.
Sure, I can do that; do you prefer that to exporting the null transformation context? (Just checking, you couldn't make it to the meeting last Friday)
There was a problem hiding this comment.
I don't have an issue with making nullTransformationContext public, but I think the approach of just passing undefined works just as well.
| @@ -1642,12 +1641,12 @@ export function transformES2015(context: TransformationContext): (x: SourceFile | |||
| case SyntaxKind.PropertyDeclaration: { | |||
| const named = node as AccessorDeclaration | MethodDeclaration | PropertyDeclaration; | |||
| if (isComputedPropertyName(named.name)) { | |||
| return factory.replacePropertyName(named, visitEachChild(named.name, elideUnusedThisCaptureWorker, nullTransformationContext)); | |||
| return factory.replacePropertyName(named, visitEachChild(named.name, elideUnusedThisCaptureWorker, /*context*/ undefined)); | |||
There was a problem hiding this comment.
The uses of nullTransformationContext (and now undefined) in this file seem suspicious; all of these call sites have a context available and using it still passes tests. @rbuckton should I be "fixing" these?
| @@ -580,20 +581,20 @@ export function visitCommaListElements(elements: NodeArray<Expression>, visitor: | |||
| * @param visitor The callback used to visit each child. | |||
| * @param context A lexical environment context for the visitor. | |||
| */ | |||
| export function visitEachChild<T extends Node>(node: T, visitor: Visitor, context: TransformationContext): T; | |||
| export function visitEachChild<T extends Node>(node: T, visitor: Visitor, context: TransformationContext | undefined): T; | |||
There was a problem hiding this comment.
Also, debating whether or not visitLexicalEnvironment, visitParameterList, visitFunctionBody, and visitIterationBody need this change. Those are pretty explicitly about working with the context and within a transform, though, so I'm leaning towards no.
These functions are used by our mini dts bundler; it'd be nice to not
as anyto use them.