Support non-ascii characters by upgrading common JS core #66
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aarsilv
commented
Jan 3, 2025
| @@ -72,4 +72,4 @@ lib/ | |||
| .node-version | |||
| .python-version | |||
|
|
|||
| test/data | |||
| src/__tests__/data | |||
There was a problem hiding this comment.
Co-locating test data with tests
| rm -rf $(testDataDir) | ||
| mkdir -p $(tempDir) | ||
| git clone -b ${branchName} --depth 1 --single-branch ${githubRepoLink} ${gitDataDir} | ||
| cp ${gitDataDir}rac-experiments-v3.json ${testDataDir} | ||
| cp -r ${gitDataDir}assignment-v2 ${testDataDir} | ||
| cp -r ${gitDataDir}ufc ${testDataDir} |
There was a problem hiding this comment.
Updating the test data to UFC
| @@ -51,7 +51,7 @@ | |||
| "registry": "https://registry.npmjs.org/" | |||
| }, | |||
| "dependencies": { | |||
| "@eppo/js-client-sdk-common": "4.3.0", | |||
| "@eppo/js-client-sdk-common": "^4.7.1", | |||
There was a problem hiding this comment.
🔧 Core of the change
| @@ -105,7 +113,9 @@ describe('EppoReactNativeClient integration test', () => { | |||
| const mockConfigStore = td.object<EppoAsyncStorage>(); | |||
| const mockLogger = td.object<IAssignmentLogger>(); | |||
| td.when(mockConfigStore.get(flagKey)).thenReturn(null); | |||
| const client_instance = new EppoReactNativeClient(mockConfigStore); | |||
| const client_instance = new EppoReactNativeClient({ | |||
| flagConfigurationStore: mockConfigStore, | |||
There was a problem hiding this comment.
Signature of the core repository constructor has changed from an argument list to an options object
| @@ -227,3 +241,63 @@ describe('EppoReactNativeClient integration test', () => { | |||
| expect(EppoReactNativeClient.initialized).toBe(false); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('UFC Obfuscated Test Cases', () => { | |||
| @@ -0,0 +1,101 @@ | |||
| import * as fs from 'fs'; | |||
There was a problem hiding this comment.
This file is largely taken from the node repository. Future us should just export this common functionality in the common module as well.
| @@ -59,13 +60,15 @@ export class EppoAsyncStorage | |||
| private environment: Environment | null = null; | |||
| private configFetchedAt: string | null = null; | |||
| private configPublishedAt: string | null = null; | |||
| private format: FormatEnum | null = null; | |||
There was a problem hiding this comment.
Updated common repository now has a format property for IConfigurationStore
|
@aarsilv is this ready for review? still in draft mode |
felipecsl
approved these changes
Jan 3, 2025
| @@ -105,7 +113,9 @@ describe('EppoReactNativeClient integration test', () => { | |||
| const mockConfigStore = td.object<EppoAsyncStorage>(); | |||
| const mockLogger = td.object<IAssignmentLogger>(); | |||
| td.when(mockConfigStore.get(flagKey)).thenReturn(null); | |||
| const client_instance = new EppoReactNativeClient(mockConfigStore); | |||
| const client_instance = new EppoReactNativeClient({ | |||
| flagConfigurationStore: mockConfigStore, | |||
| public static instance: EppoReactNativeClient = new EppoReactNativeClient({ | ||
| flagConfigurationStore: asyncStorage, | ||
| isObfuscated: true, | ||
| }); |
There was a problem hiding this comment.
much better without all those undefineds
|
@aarsilv I tested this out in RN 0.72 and it works. Before After |
greghuels
approved these changes
Jan 6, 2025
leoromanovsky
approved these changes
Jan 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Eppo Internal:
🎟️ Ticket: FF-3769 - Update React Native SDK to use latest common JS module to support non-ascii characters
🗨️ Slack Thread: "...flag text encoding problem..."
👯 Related PR:
js-sdk-common #187Motivation and Context
SDKs need to be able to handle special, non-ASCII characters.
Description
We update our common module, which switches from using
js-base64'sbtoaPolyfill()andatobPolyfill()toencodeanddecoderespectively.The former is designed only to handle ASCII characters and bytes (docs), while the latter is designed for strings and can handle all characters.
I've also added the shared test suite to our tests
How has this been tested?
Updated shared test case
Tested with the latest version of React Native, 0.76
Tested with React Native version 0.72, which had issues in the past (see FF-2139 Fix base64 encoding/decoding for React Native 0.72.x js-sdk-common#75)
