build: use yarn@4, update deps, use react-native-test-app for example (w/rn82) #814
Conversation
|
To view this pull requests documentation preview, visit the following URL: docs.page/invertase/react-native-google-mobile-ads~814 Documentation is deployed and generated using docs.page. |
mikehardy
force-pushed
the
update-example-script
branch
from
5100907 to
186fe70
Compare
| type NativeAdContextType = { | ||
| nativeAd: NativeAd; | ||
| viewRef: RefObject<React.ElementRef<typeof GoogleMobileAdsNativeView>>; | ||
| viewRef: RefObject<React.ComponentRef<typeof GoogleMobileAdsNativeView> | null>; |
There was a problem hiding this comment.
this line is problematic - the type change is potentially breaking, need to either handle differently or verify not breaking
not sure if types are included in react now but they are in react-native and I don't think I'm using the built-in types yet, I think I'm still using DefinitelyTyped ones
There was a problem hiding this comment.
@dylancom I can't see a way around this one - our internal logic actually uses it's possible null-ness, I think we just need to issue it as a lightly breaking change.
react-native-google-mobile-ads/src/ads/native-ad/NativeAsset.tsx
Lines 47 to 50 in 8a5b6ef
I'm going to re-push with the commit altered so it will trigger the correct breaking change release and notes - I can also undo that if you disagree, but you'll get to see what I'll put in there for a final approval pre-merge + release
what do you think?
|
|
||
| class BannerTest implements AutoExecutableTest { | ||
| bannerRef: RefObject<BannerAd>; | ||
| bannerRef: RefObject<BannerAd | null>; |
There was a problem hiding this comment.
this appears to be the possible impact of that null change prior as I forward-ported to modern types
…NativeAds BREAKING CHANGE: the ref in some ads *may* be null, a slight type change from stricter checks This is from upgrading all our deps with newer versions of react, react-native, and typescript Apparently they have stricter type checking as this ref was always possibly null, but it was not reflected in the inferred type passed to consuming code. Now it is
mikehardy
force-pushed
the
update-example-script
branch
from
1a84a33 to
1be7404
Compare
- no longer using detox, all the tests were manual anyway - react-native 0.82.1 is current version, used here - new types for react-native meant some module typings changed - ios-config.sh needs one more lookup iteration for react-native-test-app (but I searched for the specific sub-dir rather than change general lookup count, backwards-compatible)
- new config style - mocha/recommended doesn't exist anymore - new versions means new little formatting tweaks
an exhaustive conditional always results in the last one being unnecessary in a strongly typed system since it must be true by process of elimination of the other enum elements in the type system, this results in a lint error with strict checking a more reliable way to test if the arg is in the enum, that does not result in an unnecessary conditional is to check if the arg is in the enum as a JS object
… -> Ref codegen still requires ElementRef though, due to upstream issue
- loaded status wasn't handled well in class components for the 4 loadable tests - can only useState in functional components, and that's the easiest, so convert - all of the 4 different loadable tests were doing basically the same thing though, so make a more generic loadable component test and have the 4 just implement the differences
this simplifies the native modules there and should also speed up app startup time as constants won't have to be loaded from native dynamically during module init - they are equivalent across platforms, so no technical reason to load from native - none of the other constants like this are loaded from native
- requires to provide an eslint parser source for types - modified one included rule slightly to allow numbers in template strings
not sure why this was an NSNumber* before, with mismatched types and conversions...
it seemed that they were all non-null?
…consent you need to inform the Xcode compiler that you are not intending to reimplement the setters/getters for the property (i.e., '@synthesize'), you just want a narrower type, so you need to mark them as '@dynamic'
mikehardy
force-pushed
the
update-example-script
branch
from
2b9a543 to
59dc53f
Compare
|
Fantastic job! Lot's of improvements in there and with a modern example app we can easier adapt the final pieces to the new arch. |
|
🎉 This PR is included in version 16.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Gets the example app up to date, and all our internal deps + tooling at the same time
Works for me locally?