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

improvement: upgrade typescript and target #10676

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Klescouar
Copy link

Upgrade TypeScript from version 5.3.2 to 5.6.2 and change TypeScript target to “ESNext”

This PR enhances our project by upgrading TypeScript from version 5.3.2 to 5.6.2. This update allows us to take advantage of the latest features, optimizations, and bug fixes provided by TypeScript.

Changes Made

TypeScript Upgrade:

TypeScript has been updated from 5.3.2 to 5.6.2. This includes several performance improvements and new features, such as support for named capturing groups and type enhancements.

Type Errors Correction:

All type errors introduced by this update have been fixed to ensure compatibility and maintain code quality.

TypeScript Target Change:

The target has been changed in the tsconfig.json file from "es5" to "ESNext". This allows us to use modern JavaScript features, making the code cleaner and more efficient.

Copy link

vercel bot commented Oct 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencollective-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2024 9:47am
opencollective-styleguide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2024 9:47am

Copy link
Member

@Betree Betree left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. There's one blocker on the compilation target and currencyDisplay can be simplified, the rest looks good!

tsconfig.json Outdated
@@ -1,6 +1,6 @@
{
"compilerOptions": {
"target": "es5",
"target": "ESNext",
Copy link
Member

@Betree Betree Oct 14, 2024

Choose a reason for hiding this comment

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

The justification you provide for this change:

The target has been changed in the tsconfig.json file from "es5" to "ESNext". This allows us to use modern JavaScript features, making the code cleaner and more efficient.

Is pretty unclear. Which features? What's cleaner? What's more efficient? The target option impacts the final build. As per the docs:

Modern browsers support all ES6 features, so ES6 is a good choice. You might choose to set a lower target if your code is deployed to older environments, or a higher target if your code is guaranteed to run in newer environments.

The target setting changes which JS features are downleveled and which are left intact. For example, an arrow function () => this will be turned into an equivalent function expression if target is ES5 or lower.

Changing target also changes the default value of lib. You may “mix and match” target and lib settings as desired, but you could just set target for convenience.

For developer platforms like Node there are baselines for the target, depending on the type of platform and its version. You can find a set of community organized TSConfigs at tsconfig/bases, which has configurations for common platforms and their versions.

The special ESNext value refers to the highest version your version of TypeScript supports. This setting should be used with caution, since it doesn’t mean the same thing between different TypeScript versions and can make upgrades less predictable.

So this change introduces a risk of breaking the website for older browser. If we ever do it, there must be a good justification and it must be properly tested for regressions.

Suggested change
"target": "ESNext",
"target": "es5",

lib/currency-utils.ts Outdated Show resolved Hide resolved
lib/currency-utils.ts Outdated Show resolved Hide resolved
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.

3 participants