Skip to content
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

Isolated declarations improvements to triple slash directives and imports. #94

Merged

Conversation

dragomirtitian
Copy link

@dragomirtitian dragomirtitian commented Sep 7, 2023

No description provided.

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
…invalid code.

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
@dragomirtitian dragomirtitian force-pushed the isolated-declarations-improvements branch 2 times, most recently from cf61980 to 99ce432 Compare September 8, 2023 09:53
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
…fied with the codemod

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
@dragomirtitian dragomirtitian force-pushed the isolated-declarations-improvements branch from 99ce432 to e772096 Compare September 8, 2023 09:56
Copy link

@h-joo h-joo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just some questions to ask for clarifications.

newLine: settings.newLine,
target: settings.target,
removeComments: false,
omitTrailingSemicolon: true,
Copy link

Choose a reason for hiding this comment

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

Just curious, why have you set this?

Copy link
Author

@dragomirtitian dragomirtitian Sep 11, 2023

Choose a reason for hiding this comment

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

It created some print differences with original tests

@@ -54,7 +54,7 @@ function findNearestParentWithTypeAnnotation(node: ts.Node): ts.Node | undefined
export function addTypeAnnotationTransformer(sourceFile: ts.SourceFile, program: ts.Program, moduleResolutionHost?: ts.ModuleResolutionHost) {
const typeChecker = program.getTypeChecker();
const nodesToFix = new Map(program.getDeclarationDiagnostics(sourceFile).
filter((diag) => diag.code === 9007).
filter((diag) => diag.code === 9007 || diag.code === 9009).
Copy link

Choose a reason for hiding this comment

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

I'm guessing I should write a fix for the new error code 9009 by adding declarations to property assignments happening in functions?

Copy link
Author

Choose a reason for hiding this comment

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

My intention is to use the new error code for both function declarations and expressions. But yes, it would be helpful to treat the error for expando function declarations.

if(node.initializer && localType && strictNullChecks && !resolver.isOptionalParameter(node)) {
localType.typeNode = addUndefinedInUnion(localType.typeNode);
else {
localType = invalid(node);
Copy link

Choose a reason for hiding this comment

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

Can you clarify what this chunk of changes imply? I don't think I understood.

Copy link
Author

Choose a reason for hiding this comment

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

I initially wanted to support types parameters that are not annotated (ok under noImplicitAny: false). Turns out that has several complications so I decided to revert the support for those.

@dragomirtitian dragomirtitian merged commit f1f82f9 into isolated-declarations Sep 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants