Skip to content

Conversation

@TrCaM
Copy link
Collaborator

@TrCaM TrCaM commented May 23, 2019

Description

  • Add readonly to all properties of types and interfaces declared in index.d.ts

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

In general we shouldn't mark fields of input types readonly. I've pointed out several of those instances, but there are more. Lets only mark fields of output types as readonly.

@hiranya911 hiranya911 assigned TrCaM and unassigned hiranya911 May 24, 2019
@TrCaM
Copy link
Collaborator Author

TrCaM commented May 27, 2019

@nbegley, @hiranya911, I reverted the readonly on the types that are used as input arguments for functions, thus allow user to modify those objects if necessary. Please check if my update is reasonable, and if I need to add/remove readonly on some other fields.
Thanks!

@nbegley nbegley assigned hiranya911 and unassigned TrCaM May 27, 2019
@TrCaM TrCaM requested a review from hiranya911 May 28, 2019 18:29
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one question to @nbegley.

@hiranya911 hiranya911 assigned nbegley and unassigned hiranya911 May 28, 2019
@hiranya911 hiranya911 requested a review from nbegley May 28, 2019 20:44
@nbegley
Copy link
Contributor

nbegley commented May 28, 2019

We'll probably have to consider this a breaking change. Even though users shouldn't be relying on any of these fields being mutable, it's possible that some of them may be.

@nbegley nbegley assigned hiranya911 and unassigned nbegley May 28, 2019
@hiranya911
Copy link
Contributor

I agree @nbegley. On a second thought, I'm even reluctant to do this change for output types. Somebody may have written code like this:

const user = await admin.auth().getUser(uid);
user.customClaims = {foo: 'bar'};
await admin.auth().setCustomUserClaims(uid, user.customClaims);

Having said that I do believe this is a good change, and something we ought to do at some point (possibly for the next major release). @TrCaM can you create an issue for this, so we will remember to revisit it in the future?

@TrCaM
Copy link
Collaborator Author

TrCaM commented May 30, 2019

Sure, I can add an issue

@hiranya911
Copy link
Contributor

Thanks. We will track this requirement via the internal issue you've reported, and get it done for the next major release.

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