feat(nativescript): v7 updates and compatibility to target es2017+

[NathanWalker]

• [x] updates to latest rc's to prepare codebase
• [x] suggests @nativescript/firebase for future home (already rc published there)
• [x] verifying all api's work as expected

[james-criscuolo]

@NathanWalker I don't really know the reasoning, but it looks like the postinstall script that sets up the config lives in two places, one in src, one in publish. The publish one hasn't been updated, and I believe it is used to build the src one. Here is the file: https://github.com/nstudio/nativescript-plugin-firebase/blob/feat/dep-updates/publish/scripts/installer.js#L16

If I do npm run package from src, I get a diff wiping away some of your (necessary) changes.

[james-criscuolo]

@NathanWalker I see that rc.5 is published on npm but is not updated on this branch. Is it somewhere else so I can rebase on top of it? I've got my Android build working on the rc stuff, and it appears this dep is my current blocker on ios.

[NathanWalker]

Thanks @james-criscuolo sorry for late reply - latest is pushed up now but there's some more typings to clean up which we can help with this week. Please let me know if you're able to pull the latest here.

[james-criscuolo]

I was able to pull the latest, just having issues getting this running. I notice when npm run package is run, a few dozen errors spit out that do not prior to your commits. Can you write up how the code is testable at this point? Prior to the latest commits, i was able to package and run something on android, but with the latest commits I get a seemingly useless error:

Failed to build plugin @nativescript/firebase :
Error: spawn ./gradlew EACCES

If the code is testable in some way, I'll get my commits on top and use the test to get to the bottom of it. If you do think it's related to the errors on package, I can take a look at that as well. For reference, my commits are a different PR (#1111), but I think I've been able to rebase it cleanly.

[EddyVerbruggen]

@james-criscuolo Just FYI I got rid of that "duplicate" postinstall script in master. It was there because years ago I wanted folks to be able to pull the latest from Github directly (as opposed to npm) to test bleeding edge versions. That required the webpacked version op the master script in /publish. Since this got converted to TS it wasn't of much use (as you can't pull that into node_modules directly anyway).

[NathanWalker]

@james-criscuolo thanks for posting in a new PR - we'll see if can pull your fork down and help from that point.

[james-criscuolo]

Sure thing, and it would be great if that PR was taken, but all of my questions are from the point of this PR. If I could get this PR up and running (like with the demo commands in this repo), I'd be happy to continue pulling along those changes if necessary.

[NathanWalker]

@james-criscuolo Shooting to get the typings squared out by eod Thursday -- The main thing is to allow nice/succinct symbols for each aspect of the firebase plugin to provide for good tree shaking of things not used - hence the switch here to import { firebase } vs. import * as firebase - to achieve that all way around some shifting of the structure and setup was needed.

[NathanWalker]

ok @james-criscuolo build is fixed now - please give it a go and lemme know how things look from your end with this.

[james-criscuolo]

I get no build errors now, but my apps do not get further along during their build process. I did notice one warning pop up that a change to firebase-common.ts fixes:

 export const firestore = firebase.firestore;
+export const functions = firebase.functions;

My next step is to go back and try building and running the demos with just this PR, as I wasn't able to do that before, but may be able to now.

[NathanWalker]

@james-criscuolo there was some global handling attaching things to that firebase object throughout which we ultimately probably may wanna move away from. Rather provide distinct symbols with relevant functions for that area on them which are exported. That way import { firebase } from '@nativescript/firebase'; could tree shake out all the other stuff not used and also import { firestore, functions as fbFunctions } from '@nativescript/firebase' for instance if you were just using those things.

[james-criscuolo]

So i wasn't able to put much time in on this today, but I stumbled upon an interesting clue regardless. I had the new cli dependency installed, but was back on my stable branch. I'm aware there are a few things that occur differently, but I had been able to get past the install up until recently. I ended up running into the same issue as I ran into with this PR, just with my stable version of theis library. This leads me to believe something with the packaging is now messed up, or the CLI now reads packages differently.

What we do in our library:

• call npm run package
• copy the .tgz file spit out in the publish folder to our repo
• add as a dependency like `"nativescript-plugin-firebase": "file:../nativescript-plugin-firebase-10.5.2.tgz"

I need to read the plugin authors blog, but I'm hoping the change needed (at least, to get over this hurdle) has nothing to do with the specifics of this repo.

[NathanWalker]

Really appreciate the assistance @james-criscuolo - daily client work has been heavy - perhaps would be helpful if we did a Zoom on Thursday to look at things together to help clear any remaining issues.

[james-criscuolo]

Ok so I did some digging and I noticed that my gradlew EACCESS error is all over google, and the solution is to chmod gradlew. In platforms/tempPlugin/nativescript-plugin-firebase, the latest version of nativescript (7.0.6 I believe) leaves this:

-rw-r--r--   1 jamescriscuolo  staff   5.2K Sep  9 14:33 gradlew

And installing via nativescript@6 (6.8.0) leaves

-rwxr-xr-x   1 jamescriscuolo  staff   5.2K Sep  9 14:56 gradlew

As I wanted to test the new version after chmod'ing, That install downloaded a new version of gradle and I see

Set executable permissions for: /Users/jamescriscuolo/.gradle/wrapper/dists/gradle-6.4-bin/aj6cyggqps6mdbpl6cfppfwqk/gradle-6.4/bin/gradle

I think Android Studio may have updated gradle instead of in the nativescript build process? I was able to get past this, but not sure how I ended up in this situation to begin with.

So I think I'm past any issues with Android, I have an issue now related to the introduction of the new configuration file, but haven't dug in enough to get to the bottom of it yet. Once I get through that one, I plan to hop back to iOS and see what is happening there.

[james-criscuolo]

It would appear the new build system expects package.json to be in a child of appPath. My repo is set up like this

                     folder
              /.                          \
{N} project files           src code for {N}, others

So my only package.json for nativescript is now in the {N} project files folder, with the new nativescript.config.ts which has an appPath of ../src. The new system was finding other package.jsons I have (for other projects sharing code), and re-adding a package.json in src (where the one with main used to be) also did not work.

I recognize this is not related to this issue at all. I'm happy to open a new issue, and create a test repo for this. It should be pretty easy to replicate, you end up with a max stack size exceeded if no package.json's exist.

To get past this for the time being, I'm going to attempt switching back to the old configuration method.

[james-criscuolo]

So my android app now runs and firebase seems to be functional. I think iOS had been blocked by the same nativescript.config.ts issue highlighted above, but now that I've tabled that temporarily, I have a new issue:

CONSOLE ERROR: UNCAUGHT ERROR HANDLER: ReferenceError: NativeClass is not defined
CONSOLE ERROR: UNCAUGHT ERROR HANDLER: *** STACK TRACE BEGIN ***
CONSOLE ERROR:  at ReferenceError: NativeClass is not defined
CONSOLE ERROR:  at     at Module.../nativescript/OnSIP/node_modules/@nativescript/firebase/admob/admob.js (file:///app/vendor.js:143209:1)
CONSOLE ERROR:  at     at __webpack_require__ (file:///app/runtime.js:817:30)

I have NativeClass elsewhere in my code, so I'm not sure why the package can not access it.

[NathanWalker]

Excellent investigations @james-criscuolo - I can help on the NativeClass - is your latest pushed up here to this branch or can let me know what fork/branch I can pull - I can clear that last hurdle momentarily.

[james-criscuolo]

I've been using this branch, with #1111 rebased on top, but just fixing this branch should be enough for me to retest once this is cleared.

[NathanWalker]

@james-criscuolo ok just pushed up what should help your case. The NativeClass is not defined error occurs if ts-patch did not install and thus the transformer did not apply to the tsc output. After pulling latest here, run npm run clean and then npm run tsc and it should properly apply transformations. Lemme know if looking better after doing that.

[james-criscuolo]

Wanted to give a brief update here. I was able to get past my build issue after your most recent commit. My iOS build is now stuck trying to get the AppDelegate to work. Comparing it to our old sample repo with an AppDelegate (which works), I'm trying to narrow down the differences, as I'm not sure what else could cause it. Is there anything that won't work with a NativeClass? Our AppDelegate has static functions and variables declared in the file outside of the class, which the sample one does not have. I'm working on pulling those out, so I'm hoping to get over this hurdle tomorrow.

[NathanWalker]

Let's do a zoom @james-criscuolo to look together - lemme know a good time on Friday or Monday 👍

[james-criscuolo]

I can do anytime after 12PM eastern time tomorrow or Monday, just let me know.

[jalberto-ghub]

Hi guys, I just tried rc 6 on Android and it is still broken. You cannot login at all. The following code on firebase.android.js line 779 is broken:

firebase.login = arg => {
    return new Promise((resolve, reject) => {
        try {
            this.resolve = resolve;
            this.reject = reject;
....

I do not think you can use this outside a class definition. The iOS version does not do this. This is the error I get when trying to login:

JS: ERROR Error: Uncaught (in promise): TypeError: Cannot set property 'resolve' of undefined
JS: TypeError: Cannot set property 'resolve' of undefined
JS:     at file: node_modules\@nativescript\firebase\firebase.android.js:779:16
JS:     at new ZoneAwarePromise (file: node_modules\@nativescript\zone-js\zone-nativescript.js:902:0)
JS:     at Object.push.../node_modules/@nativescript/firebase/firebase.js._firebase_common__WEBPACK_IMPORTED_MODULE_2__.firebase.login (file: node_modules\@nativescript\firebase\firebase.an
droid.js:777:0)

[NathanWalker]

@jalberto-ghub we just published rc.7 (if using rc in your package.json, a clean/rebuild will pick it up) - should fix that android resolve issue - lemme know when get a chance 👍

[Klunk75]

@jalberto-ghub we just published rc.7 (if using rc in your package.json, a clean/rebuild will pick it up) - should fix that android resolve issue - lemme know when get a chance

@NathanWalker, I had this issue on Android as well, and I confirm it's now fixed (by rc.7). Thanks a lot!

Paolo

[shipley-dcc]

Hi @NathanWalker, I think a bug has crept into the Firestore Document Reference object. The firebase.android.ts file now has the following function:

`firebase.firestore._getDocumentReference = (docRef?: JDocumentReference): firestore.DocumentReference => { if (!docRef) { return null; }

const collectionPath = docRef.getParent().getPath();

return { discriminator: "docRef", id: docRef.getId(), parent: firebase.firestore._getCollectionReference(docRef.getParent()), path: docRef.getPath(), firestore: firebase.firestore, collection: cp => firebase.firestore.collection(${collectionPath}/${docRef.getId()}/${cp}), set: (data: any, options?: firestore.SetOptions) => firebase.firestore.set(collectionPath, docRef.getId(), data, options), get: (options?: firestore.GetOptions) => firebase.firestore.getDocument(collectionPath, docRef.getId(), options), update: (data: any) => firebase.firestore.update(collectionPath, docRef.getId(), data), delete: () => firebase.firestore.delete(collectionPath, docRef.getId()), onSnapshot: (optionsOrCallback: firestore.SnapshotListenOptions | ((snapshot: firestore.DocumentSnapshot) => void), callbackOrOnError?: (docOrError: firestore.DocumentSnapshot | Error) => void, onError?: (error: Error) => void) => firebase.onDocumentSnapshot(docRef, optionsOrCallback, callbackOrOnError, onError), android: docRef }; };`

The onSnapshot function now has a reference to firebase.onDocumentSnapshot instead of firestore.onDocumentSnapshot which is giving a TypeError and stopping the onSnapshot call from working.

Thanks. Dave

[NathanWalker]

@shipley-dcc thank you - we'll correct this and get another rc out or may publish a final 11.0.0 version.

[NathanWalker]

@shipley-dcc rc.8 was just published - if using rc in your package you can just clean/rebuild - could you let us know if that clears it for you? In 2-3 hours we'll publish a final on this.

[shipley-dcc]

@NathanWalker That's fixed it. Thanks.

[NathanWalker]

Excellent thanks - we just published 11.0.0 final with {N} 7 compatibility. If anything else comes up ping here in time being and we can help publish a patch if needed.

[bellalMohamed]

Any information about when will the PR get merged?

[NathanWalker]

@bellalMohamed we’re discussing with @EddyVerbruggen whether we drop this in a workspace for the scoped 11.0 ({N} 7 compat) and beyond future. There’s a number of good reasons to not merge this here actually. For quite some time if not infinitely there will always be older projects out there on {N} 6 and lower so this can and will always serve as the commonjs/es5 base. Another reason is with {N} 7 and beyond the benefits of the plugin workspace are immense with future maintainability for a plugin this large. I would anticipate some decision being made here over the next 2 weeks.

Another option is to branch master to tns-core-modules and merging to master and adding a more workspace style setup here.

The shift from es5 > es2017 is significant and one of the core reasons behind scoping some high use plugins to allow both ‘latest’ versions to exist on either providing some clarity to {N} 6 and below vs {N} 7 and beyond.

[bellalMohamed]

My humble opinion, is to drop 11.0 in a workspace and set a depreciation date for the {N} 6 support, as the new mindset of {N} 7 is to stay up-to-date, so I know it will be hard to drop support for older projects, but it's inevitable, and too focus more on the future of {N}, and give a better support for these packages.

[Mc-Mido]

Hi all,

Thanks for your MR 👍

Are these RC versions available somewhere (I can't find it on npm) ?

Do you know when is the version 11.0 planned ?

Edit : ok find it => https://www.npmjs.com/package/@nativescript/firebase

[romandrahan]

@EddyVerbruggen, @NathanWalker,

I'm getting a random crash when trying to login with Google on iOS after upgrade to NS 7 and plugin 11. Sometimes it's fine, sometimes it's crashing.

====== Assertion failed ======
Native stack trace:
1          0x11104119f tns::Assert(bool, v8::Isolate*) + 119
2          0x110fa2259 tns::ArgConverter::Invoke(v8::Local<v8::Context>, objc_class*, v8::Local<v8::Object>, tns::V8Args&, tns::MethodMeta const*, bool) + 95
3          0x111002e5a tns::MetadataBuilder::InvokeMethod(v8::Local<v8::Context>, tns::MethodMeta const*, v8::Local<v8::Object>, tns::V8Args&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool) + 76
4          0x1110029ae tns::MetadataBuilder::PropertyGetterCallback(v8::FunctionCallbackInfo<v8::Value> const&) + 246
5          0x11118e13c v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) + 620
6          0x11118d5ec v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 556
7          0x11118cf94 v8::internal::Builtins::InvokeApiFunction(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::HeapObject>) + 692
8          0x11152ddd2 v8::internal::Object::GetPropertyWithAccessor(v8::internal::LookupIterator*) + 466
9          0x11152d51d v8::internal::Object::GetProperty(v8::internal::LookupIterator*, bool) + 125
10         0x11138fca0 v8::internal::LoadIC::Load(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Name>, bool) + 1632
11         0x1113988ce v8::internal::Runtime_LoadNoFeedbackIC_Miss(int, unsigned long*, v8::internal::Isolate*) + 286
12         0x111a25b39 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit + 57
13         0x111a99430 Builtins_LdaNamedPropertyHandler + 4080
14         0x1119bec42 Builtins_InterpreterEntryTrampoline + 194
JavaScript stack trace:
at <anonymous> (file: node_modules/nativescript-plugin-firebase-updated/firebase.ios.js:873:36)
at GIDSignInDelegateImpl.signInDidSignInForUserWithError (file: node_modules/nativescript-plugin-firebase-updated/firebase.ios.js:2222:13)

It seems the problem is in this line: https://github.com/nstudio/nativescript-plugin-firebase/blob/b21fa1e284c7094d5592d61f03571abd264c718b/src/firebase.ios.ts#L1179

[EddyVerbruggen]

I'm going to release one last {N} 6 compatible version (10.6.0) because of the Crashlytics deprecation deadline coming up. Afterwards I'll probably merge this PR in this repo. Still, this repo may get transferred to some other org one day, but I don't think anyone would love to maintain a pre-{N}7 version indefinitely.

[NathanWalker]

Thanks @EddyVerbruggen - I’ll update this branch with latest master including the crashlytics updates later today and publish a new patch on 11.x 👍

[EddyVerbruggen]

@NathanWalker That's awesome! I just published the changes so you can go ahead. Let me know when you're done, then I'll happily merge this PR.

[NathanWalker]

@EddyVerbruggen @nativescript/firebase@11.1.0 is published now which includes latest master (the crashlytics updates). It also updates codebase to use TypeScript 4.x 👍

[NathanWalker]

https://github.com/nstudio/nativescript-plugin-firebase/blob/b21fa1e284c7094d5592d61f03571abd264c718b/src/firebase.ios.ts#L1179

@romandragan Have not seen this but it's possible the delegate may get Garbage collected early due to fact it's locally scoped inside function:

let delegate = GIDSignInDelegateImpl.new().initWithCallback((user, error) => {

and not attached as a reference to an outside class/object. We can fix this even after a merge @EddyVerbruggen as this issue exists on master as well - safer to attach any delegate to a class or object rather than have a function scoped let variable hanging out there as in v8 engine could garbage collect those early. @romandragan which runtime are you using? @nativescript/ios 7.0.4, tns-ios 6.5.3 ?

[romandrahan]

@NathanWalker, @EddyVerbruggen, I'm on the latest @nativescipt/ios@7.0.4.

[NathanWalker]

@romandragan We published 11.1.1-rc.0, see here: https://github.com/EddyVerbruggen/nativescript-plugin-firebase/pull/1714 Could you try this version and then ns clean, and run app to see if better. Let us know on that posted PR.

[romandrahan]

@NathanWalker, sure thing, will do. Thanks!