Skip to content

Cleaned up auth user types for easier debugging - #1

Merged
jwngr merged 2 commits into
masterfrom
jw-hide-internal-vars
Mar 21, 2017
Merged

Cleaned up auth user types for easier debugging#1
jwngr merged 2 commits into
masterfrom
jw-hide-internal-vars

Conversation

@jwngr

@jwngr jwngr commented Mar 20, 2017

Copy link
Copy Markdown

Description

I’ve now seen several reports of people getting confused by the fact that when you log UserRecord, you get a bunch of properties which are named <something>Internal. People think those are what they should use instead of just <something>. This change has no functional difference other than cleaning up which these properties display when doing a console.log().

Code sample

Previous output:

UserRecord {
  uidInternal: '0aiHf8cXAhumgYbHqE05',
  emailInternal: '0aihf8cxahumgybhqe05@example.com',
  emailVerifiedInternal: false,
  displayNameInternal: 'Random User 0aiHf8cXAhumgYbHqE05',
  photoURLInternal: 'http://www.example.com/0aiHf8cXAhumgYbHqE05/photo.png',
  disabledInternal: false,
  metadataInternal: 
   UserMetadata {
     createdAtInternal: 2017-02-02T05:25:35.000Z,
     lastSignedInAtInternal: null },
  providerDataInternal: 
   [ UserInfo {
       uidInternal: '0aihf8cxahumgybhqe05@example.com',
       displayNameInternal: 'Random User 0aiHf8cXAhumgYbHqE05',
       emailInternal: '0aihf8cxahumgybhqe05@example.com',
       photoURLInternal: 'http://www.example.com/0aiHf8cXAhumgYbHqE05/photo.png',
       providerIdInternal: 'password' } ] }

New output:

UserRecord {
  uid: '0aiHf8cXAhumgYbHqE05',
  email: '0aihf8cxahumgybhqe05@example.com',
  emailVerified: false,
  displayName: 'Random User 0aiHf8cXAhumgYbHqE05',
  photoURL: 'http://www.example.com/0aiHf8cXAhumgYbHqE05/photo.png',
  disabled: false,
  metadata: UserMetadata { createdAt: 2017-02-02T05:25:35.000Z, lastSignedInAt: null },
  providerData: 
   [ UserInfo {
       uid: '0aihf8cxahumgybhqe05@example.com',
       displayName: 'Random User 0aiHf8cXAhumgYbHqE05',
       email: '0aihf8cxahumgybhqe05@example.com',
       photoURL: 'http://www.example.com/0aiHf8cXAhumgYbHqE05/photo.png',
       providerId: 'password' } ] }

jwngr
Cleaned up auth user types for easier debugging
1459f0a 
I’ve now seen several reports of people getting confused by the fact
that when you log `UserRecord`, you get a bunch of properties which are
named `<something>Internal`. People think those are what they should
use instead of just `<something>`. This change has no functional
difference other than cleaning up which these properties display when
doing a `console.log()`.

Change-Id: I79d79fa6bf7c0184d9db947031eecca5e9af1d4b
@jwngr
jwngr requested a review from bojeil-google March 20, 2017 16:53
jwngr
jwngr commented Mar 20, 2017
View reviewed changes
Comment thread src/auth/user-record.ts
export class UserMetadata {
private lastSignedInAtInternal: Date;
private createdAtInternal: Date;
public readonly createdAt: Date;

@jwngr jwngr Mar 20, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note that readonly here is a compilation time check only. I think it's useful to have to make sure we don't accidentally set the property in our own codebase, but it does not enforce read-only behavior for end-users. That is why I needed to explicitly use Object.defineProperty() below to enforce the read-only behavior.

bojeil-google
bojeil-google suggested changes Mar 21, 2017
View reviewed changes
Comment thread src/utils/index.ts Outdated
*
* @return {Object} The object that was passed to the function.
*/
export function addGetter(obj: Object, prop: string, value: any): void {

@bojeil-google bojeil-google Mar 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe this should be renamed to something like addReadonlyGetter or something of the like since a getter does not necessarily imply the property does not have a setter?

Also would be helpful to a test for this, where you add a readonly property on an object, try to change its value and then check the old value is still there.

@jwngr jwngr Mar 21, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

jwngr
Renamed helper to addReadonlyGetter() and added tests
89fe375 
Change-Id: Iddcd18cfeab45921d55f05232fad6bb1691609b2
@jwngr

jwngr commented Mar 21, 2017

Copy link
Copy Markdown
Author

Back to you @bojeil-google! Also, in the future, please use the GitHub UI to assign the PR back to me after you are done with your review. That way, you'll get a notification when I've assigned the PR back to you once I'm done, instead of me having to send you a comment like this.

@bojeil-google bojeil-google assigned jwngr and unassigned bojeil-google Mar 21, 2017
@jwngr
jwngr merged commit 0f86418 into master Mar 21, 2017
@jwngr
jwngr deleted the jw-hide-internal-vars branch March 21, 2017 19:56
@jwngr jwngr removed their assignment Apr 15, 2017
hiranya911 pushed a commit that referenced this pull request May 8, 2017
Jacob Wenger
Added contribution instructions and issue / PR templates (#1)
d56ff4f
@byrondover byrondover mentioned this pull request Aug 15, 2017
Closed
@mkonikov mkonikov mentioned this pull request Feb 10, 2020
Closed
MathBunny added a commit that referenced this pull request Aug 18, 2020
@MathBunny
Add attempt #1
f2b1924
@lahirumaramba lahirumaramba mentioned this pull request Apr 14, 2023
Merged
@fourclever fourclever mentioned this pull request Apr 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@bojeil-google bojeil-google bojeil-google approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jwngr @bojeil-google