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

feat: automatically generate declaration files #310

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

joshvermaire
Copy link
Contributor

Few things of note:

  1. There's a few extra declarations here, like NamiBridge and NamiManager and other interfaces like INami, INamiCustomerManager, etc. If these aren't expected we may need make some modifications if they're not wanted to be in the declaration output.
  2. A few others like ICampaignManager aren't exported
  3. A lot of type declarations were moved to the types.d.ts file

@joshvermaire joshvermaire requested a review from namidan May 23, 2024 19:51
Copy link
Contributor

@namidan namidan left a comment

Choose a reason for hiding this comment

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

@joshvermaire doesn't seem like the added declarations are of much consequence, since the publicly facing names seem to alias to them e.g. iNami. Does this seem right to you?

@joshvermaire
Copy link
Contributor Author

I made a script to check the differences between the two, this is what I found:

The main branch does not include the following type declarations: [
'NamiBridge',
'INami',
'NamiMLManagerBridge',
'INamiMLManager',
'RNNamiCampaignManager',
'NamiCampaignManagerEvents',
'ICampaignManager',
'RNNamiCustomerManager',
'NamiCustomerManagerEvents',
'INamiCustomerManager',
'RNNamiEntitlementManager',
'NamiEntitlementManagerEvents',
'INamiEntitlementManager',
'RNNamiManager',
'INamiManager',
'NamiPurchaseManagerBridge',
'RNNamiPurchaseManager',
'NamiPurchaseManagerEvents',
'INamiPurchaseManager',
'NamiPaywallManagerEvents',
'ServicesEnum',
'INamiPaywallManager',
'NamiSKU',
'NamiSKUType',
'AppleProduct',
'GoogleProduct',
'AmazonProduct',
'NamiCampaignRuleType',
'AccountStateAction'
]

This update_declaration_files branch is missing these declarations: [
'NamiCampaignRule',
'NamiPaywallEvent',
'NamiPaywallComponentChange'
]

NamiCampaignRule is exported under NamiCampaignRuleType which is being used so that seems fine.
NamiPaywallEvent is used in ios/NamiCampaignManagerBridge.swift and android/src/main/java/com/nami/reactlibrary/NamiCampaignManagerBridge.kt. So I will add this declaration back in but it would be good if it could be confirmed that it works properly as I'm not 100% sure how (and if) these two files interact with these types.
NamiPaywallComponentChange is not directly used in this repo outside of the NamiPaywallEvent.

I'm assuming it's ok if the type declarations that were missing from the main branch are included in new builds, but if we want to formally exclude those we would need to add that.

@joshvermaire joshvermaire marked this pull request as ready for review June 19, 2024 19:22
@joshvermaire
Copy link
Contributor Author

Here's a link to the script if you ever need it: https://gist.github.com/joshvermaire/15ccd0e0e88baaba90daac76be5ccfd5

You can run it via ts-node compare_declaration_files.ts

@joshvermaire
Copy link
Contributor Author

@joshvermaire doesn't seem like the added declarations are of much consequence, since the publicly facing names seem to alias to them e.g. iNami. Does this seem right to you?

Yes, I think that's right. I don't see it as an issue. In the future if you wanted to 'clean things up' we could make some changes to exclude them, but I don't see it causing any issues

@namidan namidan self-requested a review June 20, 2024 18:56
@namidan namidan merged commit 4637061 into main Jun 20, 2024
5 checks passed
@namidan namidan deleted the update_declaration_files branch June 20, 2024 19:56
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.

2 participants