Skip to content

Correct AuthorizationError field declarations for ES 2022 #24405

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

Merged
merged 4 commits into from
Apr 22, 2025
Merged

Correct AuthorizationError field declarations for ES 2022 #24405

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3c9718
Correct AuthorizationError field declarations for ES 2022
CraigMacomber Apr 18, 2025
c70489b
Update packages/loader/driver-utils/src/network.ts
CraigMacomber Apr 18, 2025
6aec47c
Update packages/loader/driver-utils/src/network.ts
CraigMacomber Apr 18, 2025
dc56d7d
Update packages/loader/driver-utils/src/network.ts
CraigMacomber Apr 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/loader/driver-utils/src/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,16 @@ export class AuthorizationError
implements IAuthorizationError, IFluidErrorBase
{
readonly errorType = DriverErrorTypes.authorizationError;
readonly claims?: string;
readonly tenantId?: string;

// These properties are not assigned in this class, but instead assigned in the super constructor.
// When targeting ES 2022 or later, TypeScript generates ES6 class fields for these properties.
// This overrides the own properties dynamically created by the super constructor.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo - perhaps you mean to omit "own"

But might warrant some rewording. I think you mean to say that readonly claims?: string would override super class properties. It is hard to understand because now it says declare ... which does not have that behvior.

Maybe something like:
When targeting ES 2022 or later, TypeScript generates ES6 class fields for properties listed without declare keyword and overrides the properties dynamically created by the super constructor.

Or just leave it out and amend the following to say:
To prevent undesired overriding of super class properties (when targeting ES 2022 or later),

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 specifically mean "own properties" (the same "own properties" in APIs like "Object.getOwnPropertyNames").

I specifically mentioned this, since overriding inherited properties is much more common and easier, but not what is being done here.

How about:

Suggested change
// When targeting ES 2022 or later, TypeScript generates ES6 class fields for these properties.
// This overrides the own properties dynamically created by the super constructor.
// When targeting ES 2022 or later, TypeScript would generate ES6 class fields for these properties if they did not use "declare".
// That would override the own properties dynamically created in the super constructor
// resulting in these properties always holding `undefined` instead of their desired values.

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 that is more clear: Does that address your concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

The revision works. The difficulty is just English. "the own properties" looks like a "their own properties" because "their own" is an extremely common phrase. Putting "own" in quotes would have helped make it look intentional.

// To prevent this undesired overriding,
// these are declared using `declare` to indicate this definition is only for the TypeScript typing,
// and the actual fields come from elsewhere.
declare readonly claims?: string;
declare readonly tenantId?: string;

readonly canRetry = false;

constructor(
Expand Down
Loading