Skip to content

feat(core): support TypeScript 5.3#52572

Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:ts-5.3
Closed

feat(core): support TypeScript 5.3#52572
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:ts-5.3

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 7, 2023

Updates the repo to support TypeScript 5.3 and resolve any issues. Fixes include:

  • Updating usages of TS compiler APIs to match their new signatures.
  • In TS 5.3 negative numbers are represented as PrefixUnaryExpression instead of NumericExpression. These changes update all usages to account for it since passing a negative number into the old APIs results in a runtime error.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Nov 7, 2023
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Nov 7, 2023
@ngbot ngbot bot added this to the Backlog milestone Nov 7, 2023
@crisbeto crisbeto marked this pull request as ready for review November 7, 2023 10:40
Comment on lines 7 to 9
Copy link
Member

Choose a reason for hiding this comment

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

Are these also emitted into the .d.ts files that are shipped to NPM? If so, would the reference directives require those modules to be present in a user's node_modules tree? The @types/babel__* packages aren't deps of localize, so they may not be available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't look like it. Here's what the dist/packages-dist/localize/tools/index.d.ts looks like when it's built with TS 5.3:

export { DiagnosticHandlingStrategy, Diagnostics } from './src/diagnostics';
export { checkDuplicateMessages } from './src/extract/duplicates';
export { MessageExtractor } from './src/extract/extraction';
export { ArbTranslationSerializer } from './src/extract/translation_files/arb_translation_serializer';
export { SimpleJsonTranslationSerializer } from './src/extract/translation_files/json_translation_serializer';
export { LegacyMessageIdMigrationSerializer } from './src/extract/translation_files/legacy_message_id_migration_serializer';
export { Xliff1TranslationSerializer } from './src/extract/translation_files/xliff1_translation_serializer';
export { Xliff2TranslationSerializer } from './src/extract/translation_files/xliff2_translation_serializer';
export { XmbTranslationSerializer } from './src/extract/translation_files/xmb_translation_serializer';
export { buildLocalizeReplacement, isGlobalIdentifier, translate, unwrapExpressionsFromTemplateLiteral, unwrapMessagePartsFromLocalizeCall, unwrapMessagePartsFromTemplateLiteral, unwrapSubstitutionsFromLocalizeCall } from './src/source_file_utils';
export { makeEs2015TranslatePlugin } from './src/translate/source_files/es2015_translate_plugin';
export { makeEs5TranslatePlugin } from './src/translate/source_files/es5_translate_plugin';
export { makeLocalePlugin } from './src/translate/source_files/locale_plugin';
export { ArbTranslationParser } from './src/translate/translation_files/translation_parsers/arb_translation_parser';
export { SimpleJsonTranslationParser } from './src/translate/translation_files/translation_parsers/simple_json_translation_parser';
export { Xliff1TranslationParser } from './src/translate/translation_files/translation_parsers/xliff1_translation_parser';
export { Xliff2TranslationParser } from './src/translate/translation_files/translation_parsers/xliff2_translation_parser';
export { XtbTranslationParser } from './src/translate/translation_files/translation_parsers/xtb_translation_parser';

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually nevermind, they do get emitted into some of those re-exported files. @devversion should we list these packages as dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

Even before this update, this dependency already existed (because @babel/core is imported in the .d.ts). In addition there is also the ESM explicit extension issue with localize/tools (something on the backburner).

I think we can safely add the type dependency, in case some people check the types (skipLibCheck = false). I wonder if someone has a reference to the TS commit that changed this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of these packages expose global types right? If so, then I think it should be safe to include the dependencies. I'm not very familiar with with <reference type="..." /> or how Babel interacts with @angular/localize though. @clydin, do you have any concerns with this?

Updates the repo to support TypeScript 5.3 and resolve any issues. Fixes include:
* Updating usages of TS compiler APIs to match their new signatures.
* In TS 5.3 negative numbers are represented as `PrefixUnaryExpression` instead of `NumericExpression`. These changes update all usages to account for it since passing a negative number into the old APIs results in a runtime error.
@crisbeto
Copy link
Member Author

crisbeto commented Nov 9, 2023

I've updated localize to have dependencies on @types/babel__core and @types/babel__traverse.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub November 9, 2023 21:38
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 9, 2023
Comment on lines 7 to 9
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of these packages expose global types right? If so, then I think it should be safe to include the dependencies. I'm not very familiar with with <reference type="..." /> or how Babel interacts with @angular/localize though. @clydin, do you have any concerns with this?

@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 94096c6.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 10, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
Updates the repo to support TypeScript 5.3 and resolve any issues. Fixes include:
* Updating usages of TS compiler APIs to match their new signatures.
* In TS 5.3 negative numbers are represented as `PrefixUnaryExpression` instead of `NumericExpression`. These changes update all usages to account for it since passing a negative number into the old APIs results in a runtime error.

PR Close angular#52572
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants