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

Bump Typescript to 3.8 #3690

Closed
wants to merge 3 commits into from
Closed

Conversation

1999
Copy link
Contributor

@1999 1999 commented Jun 16, 2021

This is a blocker for #3669 as one of the TS dependencies has d.ts file with export type Smth which is only available since 3.8: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html

@1999 1999 requested a review from kamilogorek as a code owner June 16, 2021 10:02
@1999
Copy link
Contributor Author

1999 commented Jun 16, 2021

@kamilogorek hey, not sure if I fully get what's wrong in this PR that so many tests are failing.
This error message is especially suspicious:

@sentry/browser: 16 06 2021 10:46:18.121:ERROR [karma-server]: Server start failed on port 9876: Error: No provider for "framework:karma-typescript"! (Resolving: framework:karma-typescript)

Can you help me understand the issue?

@@ -19937,6 +19937,11 @@ typescript@3.7.5:
resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.7.5.tgz#0692e21f65fd4108b9330238aac11dd2e177a1ae"
integrity sha512-/P5lkRXkWHNAbcJIiHPfRoKqyd7bsyCma1hZNUGfn20qm64T6ZBlrzprymeu918H+mB/0rIg2gGK/BXkhhYgBw==

typescript@3.8.3:
version "3.8.3"
resolved "https://packages.atlassian.com/api/npm/npm-remote/typescript/-/typescript-3.8.3.tgz#409eb8544ea0335711205869ec458ab109ee1061"
Copy link
Contributor

Choose a reason for hiding this comment

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

😶

@kamilogorek
Copy link
Contributor

There's a reason why we have this version locked-in, see:
https://github.com/getsentry/sentry-javascript/blob/master/scripts/verify-packages-versions.js
#2848

I just verified the ember package mentioned in #2848 and it now compiles correctly 🤔
However, this update will still introduce a new syntax, that is not backward compatible, have to double-check everything here, as we had a lot of broken builds due to TS version bumps in the past...

@1999
Copy link
Contributor Author

1999 commented Jun 16, 2021

@kamilogorek thank you for spending time to explain this. I totally get the problem. Building something which will be used by so many people in different environments is hard.

It looks like updating to 3.9 is not possible. What do you think about updating to 3.8? Will this still be unsafe?

have to double-check everything here

Is there something I can help you with here?

@kamilogorek
Copy link
Contributor

Is there something I can help you with here?

Thanks, appreciate that.

What do you think about updating to 3.8? Will this still be unsafe?

Yes, that's, unfortunately, the problem, because TypeScript doesn't follow semver. When we bump to 3.8, we introduce the type-only imports syntax into the mix. Then, when we then use a dependency that utilizes it, we won't catch that issue, as there will be no compilation error (because we'd use 3.8), however, people using our SDK, would have an indirect dependency on the version of types that are not 3.7 compatible and it would not allow them to compile their apps.

The biggest issue here is that it's not possible to catch this, as there's effectively no bug/issue, it's that we changed the contract between the SDK and end-users. Changing minimum requirements for the compiler would mean a breaking change for a vast number of users still stuck with 3.7 (lots of older Angular projects for example).

We are currently working on rewriting some internals of the SDK, and when it's done, we will almost certainly bump the major version of the SDK as well. And this will be the only good opportunity to change the TS compiler version as well.
We, as the whole SDK team agreed that currently there's no easy way to bump it, so this change will have to wait. Sorry about that, and thanks for all the contributions!

@1999
Copy link
Contributor Author

1999 commented Jun 17, 2021

All good. Thanks!

We are currently working on rewriting some internals of the SDK

Do you have any ETAs on when this will be released? I just want to understand what my next steps will be with #3669

@1999 1999 deleted the dsorin/typescript-3-8 branch June 17, 2021 00:27
@kamilogorek
Copy link
Contributor

Not really, as it includes a lot of research work first, so it's hard to accurately estimate.
Is there a chance that you could for example vendor these incompatible types? Or lock to a specific version? Which package is this exactly?

@1999
Copy link
Contributor Author

1999 commented Jun 17, 2021

@kamilogorek this is a great idea, I will discuss it with my colleagues.

Which package is this exactly?

This is @forge/api from #3669.

@kamilogorek
Copy link
Contributor

I don't see the codebase being available publicly, only the npm package. Is that correct?

@1999
Copy link
Contributor Author

1999 commented Jun 17, 2021

@kamilogorek pretty much. Forge runtime is not a publicly available open-source component: it is a core part of Forge platform which lets us run developers' code securely. @forge/api package contains some important TS types for my PR above.

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