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!: typescript rewrite #41

Merged
merged 34 commits into from
Jan 6, 2022
Merged

Conversation

dylancom
Copy link
Collaborator

@dylancom dylancom commented Dec 14, 2021

Ready to review

@docs-page
Copy link

docs-page bot commented Dec 14, 2021

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/react-native-google-ads~41

Documentation is deployed and generated using docs.page.

@mikehardy
Copy link
Collaborator

99 problems but weak typing isn't one

@dylancom dylancom changed the title [WIP] TS rewrite refactor: typescript rewrite Dec 15, 2021
@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #41 (0158167) into main (3689b1f) will increase coverage by 0.70%.
The diff coverage is 30.81%.

❗ Current head 0158167 differs from pull request most recent head 44741f2. Consider uploading reports for the commit 44741f2 to get more accurate results

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   19.46%   20.16%   +0.70%     
==========================================
  Files          22       24       +2     
  Lines         663      665       +2     
  Branches      166      171       +5     
==========================================
+ Hits          129      134       +5     
- Misses        416      531     +115     
+ Partials      118        0     -118     

@dylancom dylancom requested a review from mikehardy December 15, 2021 21:08
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

First of all, this is amazing. Typescript is clearly the way to go and I am really excited for this.

Second, I have at this point too many decades of experience programming to admit, but I only have a year or so of heavy typescript and no real focus on typing from scratch. Combine the two and sometimes I'm right on the mark with serious issues and sometimes I'm just missing something completely and need an explanation like for a young child :-). Apologies in advance for any young child bits 👶

Third, I am of the opinion that in general those doing the work get the last say, and in general you should stay out of their way. Which gives me a strong bias for incremental improvement, light discussion, and merging - relying on the submitter to hold themselves to high standards (which you do, by the way - just stating my style)

So with all that said, I saw some things and I noted some things. And I think with a little discussion they will either be obvious to me, or they will result in improvements, and then without too much fuss we'll merge this and boom, we'll be in typescript.

Thanks!

package.json Outdated Show resolved Hide resolved
src/AdsConsent.ts Outdated Show resolved Hide resolved
src/AdsConsent.ts Outdated Show resolved Hide resolved
src/ads/MobileAd.ts Outdated Show resolved Hide resolved
src/common/validate.ts Outdated Show resolved Hide resolved
src/types/AdsConsent.interface.ts Outdated Show resolved Hide resolved
src/types/MobileAdsModule.ts Outdated Show resolved Hide resolved
src/types/MobileAdsModule.ts Outdated Show resolved Hide resolved
src/validateAdRequestOptions.ts Outdated Show resolved Hide resolved
src/validateAdShowOptions.ts Outdated Show resolved Hide resolved
@dylancom dylancom requested a review from mikehardy December 20, 2021 13:07
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Looks great - I don't see any reason to stand in the way

My only tiny thought is I have a slight preference to continue disallowing any and then using a ts-ignore very judiciously when necessary, so that any's don't creep in. And ts-ignores should be in a lint check for eventual fixing + removal. But then you could also say, just have a list of anys (along with ts-ignores) in a lint check for removal and then add the any lint rule back. It's kind of the same, so it's a small thought - and in the end it is not actionable

I'll see if I can get this packed up as a beta release now so it's easy to install it for testing to make sure the packaging is alright

@mikehardy mikehardy changed the title refactor: typescript rewrite feat!: typescript rewrite Jan 6, 2022
@mikehardy mikehardy changed the base branch from main to beta January 6, 2022 21:48
@mikehardy mikehardy merged commit 3a8f742 into invertase:beta Jan 6, 2022
github-actions bot pushed a commit that referenced this pull request Jan 6, 2022
## [4.0.0](v3.4.0...v4.0.0) (2022-01-06)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* typescript rewrite ([#41](#41)) ([3a8f742](3a8f742))

### Bug Fixes

* **release:** allow semantic-release on beta branch ([b2d1381](b2d1381))
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mikehardy
Copy link
Collaborator

Well, I got part of what I wanted. We safely have a release that is a major version (v4) and is tagged "beta" on npmjs.com, so "latest" is still the prior v3 version.

That's all good, that's safe.

However, for some reason - and unlike react-native-fbsdk-next when I did it, it got the npmjs.com release label right but it did not tag the release version itself in package.json as '-beta1'. Why? I don't know. But version 4.0.0 is "burnt" / used, so whenever this actually releases - if we need changes - it will be as 4.0.1 or something.

Not a big deal but it's odd, and I wish it had -beta1 in the version name. But I can't figure out why it didn't and in general I don't like beta releases, I prefer testing and then just "release" releases ;-), so I gave it a short timebox to figure it out and came empty and I'm leaving it for now

It's out! :-)

@mikehardy
Copy link
Collaborator

@Mikenso + @birdofpreyru if you have a chance, your help testing v4.0.0 would be appreciated. It is currently on the @beta label in npmjs.com, so you have to install it specifically via version or via beta tag. The goal is to release this in a week or so if it seems fine, but it would be great to hear real success reports (or failure reports!). Thanks for your past help here and thanks in advance if you get a chance to check v4. Cheers

@birdofpreyru
Copy link
Contributor

Hey @mikehardy , I am a bit busy with different stuff now. Time allowing, I'll probably give it a try second half ot this week.

@Mikenso
Copy link
Contributor

Mikenso commented Jan 11, 2022

Hi @mikehardy, I would like to test as soon as I will have free time.
But from quick look - It is pleasure to see dylancom got rid from many default exports, I never understood people's love for them

@Mikenso
Copy link
Contributor

Mikenso commented Jan 16, 2022

I have tested interstitial and banner at my current project written with ts.
And also checked at just created for test purposes raw js project.
It seems to work as it were at current stable version.

@mikehardy
Copy link
Collaborator

Fantastic! Thanks very much for the feedback @Mikenso

mikehardy pushed a commit that referenced this pull request Feb 7, 2022
* refactor: re-write library in typescript
* refactor: use global undefined
* no emit
* remove explicit any
* refactor: use named exports
* use the same naming as the native class MobileAds
* git track uppercase change
mikehardy pushed a commit that referenced this pull request Feb 7, 2022
## [4.0.0](v3.4.0...v4.0.0) (2022-01-06)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* typescript rewrite ([#41](#41)) ([3a8f742](3a8f742))

### Bug Fixes

* **release:** allow semantic-release on beta branch ([b2d1381](b2d1381))
github-actions bot pushed a commit that referenced this pull request Feb 7, 2022
## [4.0.0](v3.4.0...v4.0.0) (2022-02-07)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* app open ads ([6be7d02](6be7d02))
* typescript rewrite ([#41](#41)) ([4114e4b](4114e4b))

### Bug Fixes

* **android:** no banner ads loaded in the first render ([3154579](3154579))
* Block descendant focus requests ([835bdec](835bdec)), closes [/github.com/facebook/react-native/issues/32649#issuecomment-990968256](https://github.com/invertase//github.com/facebook/react-native/issues/32649/issues/issuecomment-990968256)
* point to the right native module ([df30e7b](df30e7b))
* **release:** allow semantic-release on beta branch ([106ce63](106ce63))
* typo in native event type ([fcb911a](fcb911a))
github-actions bot pushed a commit that referenced this pull request Feb 7, 2022
## [4.0.0](v3.4.0...v4.0.0) (2022-02-07)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* app open ads ([6be7d02](6be7d02))
* typescript rewrite ([#41](#41)) ([4114e4b](4114e4b))

### Bug Fixes

* **android:** no banner ads loaded in the first render ([3154579](3154579))
* Block descendant focus requests ([835bdec](835bdec)), closes [/github.com/facebook/react-native/issues/32649#issuecomment-990968256](https://github.com/invertase//github.com/facebook/react-native/issues/32649/issues/issuecomment-990968256)
* **CHANGELOG:** remove duplicate changelog chunk ([d5e59e3](d5e59e3))
* point to the right native module ([df30e7b](df30e7b))
* **release:** allow semantic-release on beta branch ([106ce63](106ce63))
* typo in native event type ([fcb911a](fcb911a))
Star-dev325 added a commit to Star-dev325/react-native-google-mobile-ads that referenced this pull request Jun 7, 2024
## [4.0.0](invertase/react-native-google-mobile-ads@v3.4.0...v4.0.0) (2022-01-06)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* typescript rewrite ([#41](invertase/react-native-google-mobile-ads#41)) ([3a8f742](invertase/react-native-google-mobile-ads@3a8f742))

### Bug Fixes

* **release:** allow semantic-release on beta branch ([b2d1381](invertase/react-native-google-mobile-ads@b2d1381))
Star-dev325 added a commit to Star-dev325/react-native-google-mobile-ads that referenced this pull request Jun 7, 2024
## [4.0.0](invertase/react-native-google-mobile-ads@v3.4.0...v4.0.0) (2022-02-07)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* app open ads ([6be7d02](invertase/react-native-google-mobile-ads@6be7d02))
* typescript rewrite ([#41](invertase/react-native-google-mobile-ads#41)) ([4114e4b](invertase/react-native-google-mobile-ads@4114e4b))

### Bug Fixes

* **android:** no banner ads loaded in the first render ([3154579](invertase/react-native-google-mobile-ads@3154579))
* Block descendant focus requests ([835bdec](invertase/react-native-google-mobile-ads@835bdec)), closes [/github.com/facebook/react-native/issues/32649#issuecomment-990968256](https://github.com/invertase//github.com/facebook/react-native/issues/32649/issues/issuecomment-990968256)
* point to the right native module ([df30e7b](invertase/react-native-google-mobile-ads@df30e7b))
* **release:** allow semantic-release on beta branch ([106ce63](invertase/react-native-google-mobile-ads@106ce63))
* typo in native event type ([fcb911a](invertase/react-native-google-mobile-ads@fcb911a))
Star-dev325 added a commit to Star-dev325/react-native-google-mobile-ads that referenced this pull request Jun 7, 2024
## [4.0.0](invertase/react-native-google-mobile-ads@v3.4.0...v4.0.0) (2022-02-07)

### ⚠ BREAKING CHANGES

* typescript rewrite (#41)

### Features

* app open ads ([6be7d02](invertase/react-native-google-mobile-ads@6be7d02))
* typescript rewrite ([#41](invertase/react-native-google-mobile-ads#41)) ([4114e4b](invertase/react-native-google-mobile-ads@4114e4b))

### Bug Fixes

* **android:** no banner ads loaded in the first render ([3154579](invertase/react-native-google-mobile-ads@3154579))
* Block descendant focus requests ([835bdec](invertase/react-native-google-mobile-ads@835bdec)), closes [/github.com/facebook/react-native/issues/32649#issuecomment-990968256](https://github.com/invertase//github.com/facebook/react-native/issues/32649/issues/issuecomment-990968256)
* **CHANGELOG:** remove duplicate changelog chunk ([d5e59e3](invertase/react-native-google-mobile-ads@d5e59e3))
* point to the right native module ([df30e7b](invertase/react-native-google-mobile-ads@df30e7b))
* **release:** allow semantic-release on beta branch ([106ce63](invertase/react-native-google-mobile-ads@106ce63))
* typo in native event type ([fcb911a](invertase/react-native-google-mobile-ads@fcb911a))
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.

4 participants