-
-
Notifications
You must be signed in to change notification settings - Fork 648
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
feat: add amazon oncreate function to expo plugin #2862
base: main
Are you sure you want to change the base?
feat: add amazon oncreate function to expo plugin #2862
Conversation
@christianEconify Thank you for this! I have a question here. Does Expo support Amazon SDK? |
Hey, it does and Amazon even recommend using expo. I have been using this patch in my codebase already, and have confirmed IAP working on Fire TV with this plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update!
Could you kindly check the codereview done by code-review-gpt
below? You may ignore tests for now if it is too burden but I hope you can cover code refactoring.
1. Consolidating addToBuildGradle
and addToMainActivity
Logic
The logic for adding lines to files is duplicated in addToBuildGradle
and addToMainActivity
. We can refactor these into a single reusable function addLineToFile
to reduce redundancy and increase maintainability.
Refactored addLineToFile
:
const addLineToFile = (
newLine: string,
anchor: RegExp | string,
offset: number,
fileContents: string
) => {
const lines = fileContents.split('\n');
const lineIndex = lines.findIndex((line) => line.match(anchor));
if (lineIndex === -1) {
console.warn(`Anchor "${anchor}" not found, skipping modification.`);
return fileContents;
}
lines.splice(lineIndex + offset, 0, newLine);
return lines.join('\n');
};
Both addToBuildGradle
and addToMainActivity
can then reuse this function, like so:
const addToBuildGradle = (newLine: string, anchor: RegExp | string, offset: number, buildGradle: string) => {
return addLineToFile(newLine, anchor, offset, buildGradle);
};
const addToMainActivity = (newLine: string, anchor: RegExp | string, offset: number, mainActivity: string) => {
return addLineToFile(newLine, anchor, offset, mainActivity);
};
2. Handling the both
Case in modifyAppBuildGradle
The both
case needs to handle the addition of flavor dimensions for both Play Store and Amazon AppStore. Here's the updated version of modifyAppBuildGradle
:
Updated modifyAppBuildGradle
:
export const modifyAppBuildGradle = (
buildGradle: string,
paymentProvider: PaymentProvider
): string => {
switch (paymentProvider) {
case 'Amazon AppStore':
return handleAmazonAppStore(buildGradle);
case 'Play Store':
return handlePlayStore(buildGradle);
case 'both':
// Ensure flavor dimensions for both stores are added
if (!buildGradle.includes(`flavorDimensions "appstore"`)) {
const anchor = 'defaultConfig'; // Place to add the new lines
return addToBuildGradle(linesToAdd['both'], anchor, -1, buildGradle);
}
return buildGradle; // If already present, return as-is
default:
throw new Error(`Unsupported payment provider: ${paymentProvider}`);
}
};
3. Isolating Logic for Testability
To improve testability, I’ve isolated the key logic (finding line indices, adding lines) into smaller functions, which can now be easily tested independently. Here’s how you can organize them for unit testing:
Example Unit Tests:
For addLineToFile
:
describe('addLineToFile', () => {
it('should add a line at the correct position', () => {
const fileContents = 'line1\nline2\nline3';
const result = addLineToFile('newLine', 'line2', 1, fileContents);
expect(result).toContain('newLine');
});
it('should skip if anchor is not found', () => {
const fileContents = 'line1\nline2\nline3';
const result = addLineToFile('newLine', 'nonexistent', 1, fileContents);
expect(result).toBe(fileContents); // No change
});
});
For modifyAppBuildGradle
:
describe('modifyAppBuildGradle', () => {
it('should add both Amazon and Play Store flavors when paymentProvider is "both"', () => {
const buildGradle = `android {
defaultConfig {
// Some config here
}
}`;
const result = modifyAppBuildGradle(buildGradle, 'both');
expect(result).toContain('flavorDimensions "appstore"');
expect(result).toContain('missingDimensionStrategy "store", "amazon"');
expect(result).toContain('missingDimensionStrategy "store", "play"');
});
it('should return unchanged build.gradle if flavorDimensions is already present', () => {
const buildGradle = `android {
flavorDimensions "appstore"
defaultConfig {
// Some config here
}
}`;
const result = modifyAppBuildGradle(buildGradle, 'both');
expect(result).toBe(buildGradle); // No modification needed
});
});
I have updated to the 1st suggestion, but don't really get the 2nd suggestion. Seems unnecessary to me |
Sorry for the delay in reviewing! Currently, we support both providers as outlined in the documentation. I think these three cases should be addressed as GPT suggested 🤔. |
Resolves issue #2861