Skip to content
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.

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

[firebase-analytics] Allow property names to contain numbers #8391

Closed
wants to merge 2 commits into from

Conversation

frantic
Copy link

@frantic frantic commented May 20, 2020

This should bring the check more inline with the comment in Analytics.ts

Why

Fixes #8390 and adds the property name to the error message for easier debugging.

How

Fixed the regex.

Test Plan

'test'.match(/^[A-Za-z][[A-Za-z_\d]*$/);    // OK
'_test'.match(/^[A-Za-z][[A-Za-z_\d]*$/);   // Fail
'1_test'.match(/^[A-Za-z][[A-Za-z_\d]*$/);  // Fail
'a1_test'.match(/^[A-Za-z][[A-Za-z_\d]*$/); // OK

@IjzerenHein IjzerenHein removed the request for review from EvanBacon May 22, 2020 09:42
Copy link
Contributor

@IjzerenHein IjzerenHein left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for this contribution!

Can you add an entry to CHANGELOG.md in the package as well?

@@ -230,14 +230,14 @@ class FirebaseAnalyticsJS {
!userPropertyName.length ||
userPropertyName.length > 24 ||
userPropertyName[0] === '_' ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can also get rid of this line now

Suggested change
userPropertyName[0] === '_' ||

@IjzerenHein IjzerenHein changed the title Allow property names to contain numbers [firebase-analytics] Allow property names to contain numbers May 28, 2020
@frantic
Copy link
Author

frantic commented May 28, 2020

#8516 is a more complete solution.

@frantic frantic closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expo-firebase-analytics doesn't allow number in property names
3 participants