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.

Sign up for GitHub

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

Jump to bottom

deps: upgrade to typescript 4.x #2990

Merged
merged 3 commits into from
Nov 11, 2020
Merged

deps: upgrade to typescript 4.x #2990

merged 3 commits into from
Nov 11, 2020

Conversation

joshgummersall
Copy link
Contributor

@joshgummersall joshgummersall commented Nov 3, 2020 •
edited
Loading

Fixes #1441

Note: it looks like our use of antlr doesn't play nicely with the newer Typescript compiler. In a few places, skipLibCheck fixes the compilation errors. In others, unfortunately, it does not. I believe adaptive-expressions and its derivatives were the only examples where typescript@3.5.3 is required.

joshgummersall
joshgummersall commented Nov 3, 2020
View reviewed changes
libraries/botbuilder-dialogs-adaptive/src/actions/switchCondition.ts
results.push(item instanceof Case ? item : new Case(item.value, item.actions));
});
return results;
class CasesConverter implements Converter<Array<CaseInput | Case>, Case[]> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this actually more accurately describes the converter. This fixes a compilation error as well.

@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch 6 times, most recently from 9162426 to 388ac38 Compare November 4, 2020 00:28
@joshgummersall joshgummersall force-pushed the jpg/lerna-yarn-workspaces branch from e76fcba to 9de05af Compare November 4, 2020 00:31
@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch from 388ac38 to b6f5805 Compare November 4, 2020 00:31
@Danieladu
Copy link
Contributor

hi @joshgummersall About the version of antlr target. I have an idea that to use antlr4 and @types/antlr4 instead of the original antlr4ts which is not maintained in the future anymore.
The only concern is we should add the type for lexer/visitor/parser js file manually. Or could we make the "allowJs" true for the typescript build?
Looking forward to your reply.

@joshgummersall
Copy link
Contributor Author

I'm hesitant to move to allowJs. I'm also not familiar with that package so I'll have to investigate. Are the types you mentioned not expressed in @types/antlr4?

@Danieladu
Copy link
Contributor

I'm hesitant to move to allowJs. I'm also not familiar with that package so I'll have to investigate. Are the types you mentioned not expressed in @types/antlr4?

The @types/antlr4 just provide the types of the antlr SDK library without the types of the generated files(lexer/parser/listener/visitor).

@joshgummersall
Copy link
Contributor Author

Got it. I'll investigate a little more tomorrow. Maybe we can figure out a reasonable way to generate some typings.

@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch 2 times, most recently from 512d9e5 to 062595e Compare November 4, 2020 21:45
@joshgummersall joshgummersall mentioned this pull request Nov 4, 2020
Closed
@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch from 062595e to 0f91bc0 Compare November 5, 2020 23:35
@stevengum
Copy link
Member

Speaking of typings, when this PR is ready to merge, let's cut a daily build prior to merge and ask the Southworks team working with Solutions to test the SDK built with TypeScript 4.x.

They caught the typings issue last year due to different TypeScript versions being used for our SDK.

@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch 2 times, most recently from 8efef93 to b4bf0dc Compare November 9, 2020 17:27
Base automatically changed from jpg/lerna-yarn-workspaces to main November 9, 2020 20:20
@joshgummersall joshgummersall requested review from axelsrz and a team as code owners November 9, 2020 20:20
@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch from b4bf0dc to 6c3de4a Compare November 9, 2020 20:29
@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch 2 times, most recently from 502a9ce to bdd6927 Compare November 10, 2020 16:59
@joshgummersall
Copy link
Contributor Author

@stevengum I've cut a build of this (4.12.0-ts4-09c112b1af10) and tested it locally with every Typescript sample. They all compiled successfully. I'm hesitant to block merging this PR right now since all our tests (including the consumer test) are passing.

I do think we should cut a build once it lands and have Southworks test it, especially if they are doing more extensive testing than what I described. In the case that they are testing more extensively we should also codify that and run it as some kind of CI job.

@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch from ee6855a to da59ea3 Compare November 10, 2020 23:07
@joshgummersall joshgummersall changed the base branch from main to jpg/execute-propagate-flags November 10, 2020 23:07
Base automatically changed from jpg/execute-propagate-flags to main November 10, 2020 23:49
Josh Gummersall added 3 commits November 10, 2020 16:03
Bump typescript versions
7e1ff65
Overhaul BotFrameworkAdapter tests
7ce3e4e 
Admittedly a bikeshed moment, but was bothered by some opaque test
failures. This makes the tests a lot more isolated, DRY, and generaly
nicer to work with.
Package downlevel files
47b90b2
@joshgummersall joshgummersall force-pushed the jpg/typescript-upgrades branch from b9846bc to 47b90b2 Compare November 11, 2020 00:04
@joshgummersall
Copy link
Contributor Author

Note: rebased to clean up commit log. No code changes.

@joshgummersall joshgummersall merged commit 51ddc59 into main Nov 11, 2020
@joshgummersall joshgummersall deleted the jpg/typescript-upgrades branch November 11, 2020 00:48
@Batta32
Copy link

Batta32 commented Nov 24, 2020

Hi @stevengum & @joshgummersall, we successfully validated the build 4.12.0-ts4-09c112b1af10 and confirmed that it's correctly working with the TypeScript Solutions components!

@Danieladu Danieladu mentioned this pull request Jan 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevengum stevengum stevengum approved these changes

@carlosscastro carlosscastro Awaiting requested review from carlosscastro

@johnataylor johnataylor Awaiting requested review from johnataylor

@Stevenic Stevenic Awaiting requested review from Stevenic

@axelsrz axelsrz Awaiting requested review from axelsrz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Engineering] TypeScript 3.7 migration plan
4 participants
@joshgummersall @Danieladu @stevengum @Batta32