Skip to content

Remove unused internal utilities #17380

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

Merged
4 commits merged into from
Aug 9, 2017
Merged

Remove unused internal utilities #17380

4 commits merged into from
Aug 9, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jul 24, 2017

Many of the functions in utilities.ts are not used.

@ghost ghost force-pushed the utils branch 2 times, most recently from ae28cfb to 549f9fb Compare July 24, 2017 22:06
@ghost ghost requested a review from sandersn July 25, 2017 16:33
@ghost ghost mentioned this pull request Jul 25, 2017
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but you'll need to merge from master and decide whether to keep findDeclaration or replace a new usage that I added with find(symbol.declarations, ...

@@ -26,20 +26,6 @@ namespace ts {
return undefined;
}

export function findDeclaration<T extends Declaration>(symbol: Symbol, predicate: (node: Declaration) => node is T): T | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't remove this one. I use it in an upcoming change and would have to put it back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be fast enough to have getDeclarationOfKind delegate to findDeclaration ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the rewrite at the bottom of the change to find(symbol.declarations, ... is good enough since my new usage requires a cast as well.

(I also just merged the new usage, so you will see it when you merge from master.)

@@ -114,7 +114,7 @@ namespace ts {
compilerOptions.reactNamespace,
tagName,
objectProperties,
filter(map(children, transformJsxChildToExpression), isDefined),
children && mapDefined(children, transformJsxChildToExpression),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much all of our other array utils in core.ts test for the array possibly being undefined. Should we not also do that for mapDefined?

@ghost
Copy link
Author

ghost commented Aug 9, 2017

@rbuckton Good to go?

@ghost ghost merged commit 17a6f7b into master Aug 9, 2017
@ghost ghost deleted the utils branch August 9, 2017 21:38
uniqueiniquity pushed a commit to uniqueiniquity/TypeScript that referenced this pull request Aug 9, 2017
* Remove unused internal utilities

* Handle undefined input to `mapDefined`
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants