Skip to content

refactor(compiler-cli): move DelegatingPerfRecorder initialization into constructor#54834

Closed
frost-cy wants to merge 1 commit intoangular:mainfrom
frost-cy:patch-1
Closed

refactor(compiler-cli): move DelegatingPerfRecorder initialization into constructor#54834
frost-cy wants to merge 1 commit intoangular:mainfrom
frost-cy:patch-1

Conversation

@frost-cy
Copy link
Contributor

@frost-cy frost-cy commented Mar 12, 2024 •
edited
Loading

When TS output target is set to ES2022 or newer, the class fields don't get transpiled.

That causes a runtime error here in the DelegatingPerfRecorder.

This is because delegatingPerfRecorder that gets initialized here passes this.perfRecorder as argument. However, this.perfRecorder doesn't get set until the NgCompiler constructor runs.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

no change in the behavior

Issue Number: N/A

What is the new behavior?

no change in the behavior

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@frost-cy frost-cy marked this pull request as ready for review March 12, 2024 23:14
@pullapprove pullapprove bot requested a review from devversion March 12, 2024 23:14
frost-cy
frost-cy commented Mar 12, 2024
View reviewed changes
packages/compiler-cli/src/ngtsc/core/src/compiler.ts Outdated
Copy link
Contributor Author

@frost-cy frost-cy Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this.perfRecorder gets set through get perfRecorder()

frost-cy
frost-cy commented Mar 12, 2024
View reviewed changes
packages/compiler-cli/src/ngtsc/core/src/compiler.ts Outdated
Copy link
Contributor Author

@frost-cy frost-cy Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

However, the livePerfRecorder in the get perfRecorder() doesn't get set until the constructor runs.

This causes a runtime error.

@frost-cy frost-cy marked this pull request as draft March 13, 2024 00:06
@frost-cy frost-cy marked this pull request as ready for review March 13, 2024 07:19
devversion
devversion approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Our code-base in general may not be compliant with useDefineForClassFields = true. Is this the only instance you saw in G3? — we might need to switch to that as well.

We are explicitly setting useDefineForClassFields = false in our builds

@frost-cy
Copy link
Contributor Author

frost-cy commented Mar 13, 2024

Our code-base in general may not be compliant with useDefineForClassFields = true. Is this the only instance you saw in G3? — we might need to switch to that as well.

We are explicitly setting useDefineForClassFields = false in our builds

There are more cases in g3 where it's a TS error - TS2729: Property * is used before its initialization. We are working on an auto fixer to fix these.

This DelegatingPerfRecorder case is different as it's a runtime error because initialization happens through getter

@devversion
Copy link
Member

devversion commented Mar 13, 2024

@frost-cy Maybe we should chat more about the effort and how Angular can be compliant? We don't have any testing in 3P to verify that the sources are compilable with useDefineForClassFields = true.

@devversion
Copy link
Member

devversion commented Mar 15, 2024

@frost-cy Can you please rename the commit message as well? Right now, only the PR title is updated.

@frost-cy @devversion
refactor(compiler-cli): move DelegatingPerfRecorder initialization in…
fec95c6 
…to constructor

Move the initialization of class field `DelegatingPerfRecorder` into the constructor.

This fixes the error : `TypeError: Cannot read properties of undefined (reading 'eventCount')`

This is blocking the roll-out of public class.
@devversion devversion added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Mar 15, 2024
@ngbot ngbot bot modified the milestone: Backlog Mar 15, 2024
@alxhub
Copy link
Member

alxhub commented Mar 15, 2024

This PR was merged into the repository by commit b961075.

@alxhub alxhub closed this in b961075 Mar 15, 2024
alxhub pushed a commit that referenced this pull request Mar 15, 2024
@frost-cy @alxhub
refactor(compiler-cli): move DelegatingPerfRecorder initialization in…
42318e7 
…to constructor (#54834)

Move the initialization of class field `DelegatingPerfRecorder` into the constructor.

This fixes the error : `TypeError: Cannot read properties of undefined (reading 'eventCount')`

This is blocking the roll-out of public class.

PR Close #54834
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Apr 15, 2024

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 Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@devversion devversion devversion approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

3 participants

@frost-cy @devversion @alxhub