Add Firebase externals for admin & functions #22

pavlospt · 2018-04-05T07:47:38Z

Adds JS externals for Firebase SDK version 1.0.0
Resolves Add declarations for "firebase" #19

Since this is my first contribution, please let me know if I miss anything or something is wrong :)

bashor · 2018-04-06T08:15:02Z

Thank you for the contribution! We'll review it as soon as possible.

anton-bannykh

Thank you for your contribution!

There are some issues though, which prevent us from accepting the PR. Most are with general layout, but that's quite important to us, because publishing scripts rely on it. =)

At the moment we don't support nested layouts (e.g. firebase-admin and firebase-functions have to be directly in the externals folder.
Version folders correspond to different major versions (e.g. v5 for firebase-admin)
For NPM package foo@x.y.z the version in konfig.json should be x.y.0. In other words we keep major and minor versions and use out own patch version for the potential updates in kotlin external declarations. Also see https://github.com/Kotlin/js-externals#how-to-choose-version-number
There should be README.md files. Empty will do. Otherwise publishing script fails =( Maybe we should remove this requirement, it's a bit silly =(
There should be some minimal tests so that we can check at lease some API is usable (see jquery/_testShared). This is mostly a smoke test for different module system usage. Using just a few classes or functions should suffice.
Packages. External declarations should reside in package js.externals.<library-name>. Otherwise using several at the same time would be a mess.
Some type definitions seem to be missing (e.g. CloudFunction)?
Could you add headers like these? That's required for copyright reasons. Also this is an opportunity to immortalize your name =)

I hope this list doesn't intimidate you, because most of it is very easily fixable. =) Also some points aren't listed in the README (sorry!) so there is no way you could have known. =)

Thank you once again for your contribution! I really hope this goes through - there has been some demand for these declarations. =)

pavlospt · 2018-04-10T17:50:21Z

Great! Thank you for your points, will try to fix them ASAP. Will probably ping you for some things that do not look very clear at the time, but I hope i will be able to get along with most of them :)

pavlospt · 2018-04-10T18:34:35Z

I think I covered most of them. Although I have some questions about 3 & 7!

Regarding 3) what should I change? The konfig.json is in the requested format. Am I missing something else? :)

Regarding 7) I used ts2kt tool to generate the files. Is it possible that it failed to convert a definition or should I look in case I missed a file?

anton-bannykh · 2018-04-11T11:15:19Z

@pavlospt Thanks! Regarding 3) answered in slack. We probably should add that to the README.

Could be. Was it @firebase/functions-types you were converting?

pavlospt · 2018-04-11T12:29:16Z

Seems that we have a blocker issue https://github.com/Kotlin/ts2kt/issues/79

ts2kt fails to convert export declare type fields :) Would be glad to know what those fields should be converted to, in order to fix the current PR and add what's missing!

anton-bannykh · 2018-04-11T12:54:05Z

Well, it's no exactly a blocker. It should be possible to be fixed by translating that bit by hand.

anton-bannykh · 2018-04-11T12:54:42Z

externals/firebase-functions/v1/src/cloud-functions.kt

+    open var before: T = definedExternally
+    open var after: T = definedExternally
+    companion object {
+        fun <T> fromObjects(before: T, after: T): `"node_modules/firebase-functions/lib/cloud-functions".Change`<T> = definedExternally


This doesn't compile

will fix that and other occurrences like it by hand!

anton-bannykh

It looks much better now, but there are a lot of compilation errors, mostly due to incorrect references to classes (like admin.AppOptions.

In order to get the list you could run ./gradlew check. It will attempt to compile the declarations and perform a few sanity checks.

anton-bannykh · 2018-04-13T16:58:48Z

externals/firebase-admin/v5/src/index.admin.app.kt

+
+@file:Suppress("INTERFACE_WITH_SUPERCLASS", "OVERRIDING_FINAL_MEMBER", "RETURN_TYPE_MISMATCH_ON_OVERRIDE", "CONFLICTING_OVERLOADS", "EXTERNAL_DELEGATION", "NESTED_CLASS_IN_EXTERNAL_INTERFACE")
+@file:JsQualifier("admin.app")
+package package js.externals.firebase.admin.app


anton-bannykh · 2018-04-13T17:04:22Z

externals/firebase-admin/v5/src/index.admin.app.kt

+
+external interface App {
+    var name: String
+    var options: admin.AppOptions


^ doesn't compile =(
It should have been just AppOptions, coupled with an import js.externals.firebase.admin.AppOptions at the beginning of the file.

pavlospt · 2018-04-13T20:11:28Z

@anton-bannykh perfect, thanks for checking it, will resolve the issues and get back :)

pavlospt · 2018-04-13T21:33:35Z

@anton-bannykh Pushed a lot of fixes for the wrong imports. But, now I am stuck on some missing types. Checking the TS typings it seems that it imports ReadStream/WriteStream from fs, PromiseLike is provided by ES5 lib and Buffer which I am not sure why it can not find :) Looking forward to your feedback!

* Went ahead and commented parts of the definitions that I could not resolve and were a compilation blocker. * There seem to be a lot of lint warnings about @nativeGetter @nativeSetter and @nativeInvoke, not sure what their proper replacement is (this also happens to the rest of the modules in externals)

pavlospt · 2018-04-30T07:38:56Z

Hey, kind re-ping on this :) Has anyone had the time to take a look :) ?

anton-bannykh · 2018-04-30T08:30:32Z

@pavlospt Sorry, I've been on vacation. =( Will be back to work on Wednesday. Sorry for letting you hanging for so long =(

pavlospt · 2018-04-30T08:36:15Z

@anton-bannykh Ohh sorry did not know! Please take your time :)

anton-bannykh · 2018-05-03T18:15:28Z

It all looks great =)

There is a major issue though =( Fortunately the fix should be simple =)

I've tried writing a simple firebase function, following this tutorial.

When I tried using initializeApp function, the resulting JS code for CommonJS module system didn't contain the require(firebase-functions). You can see that by running ./gradlew check and viewing externals/firebase-admin/v5/build/classes/kotlin/testCommonJS/v5_testCommonJS.js file (it contains the compilation result of test.kt with module kind "commonjs").

It seems some @JsModule annotations are missing. I think index.admin.kt and index.admin.credential.kt should contain annotations

@file:JsNonModule
@file:JsModule("firebase-admin")

Something similar should be done for firebase-functions.

Thanks!

P.S. Once again, sorry for the long absence.

anton-bannykh · 2018-05-03T17:33:51Z

externals/firebase-admin/v5/src/firestore.FirebaseFirestore.kt

@@ -0,0 +1,182 @@
+@file:Suppress("INTERFACE_WITH_SUPERCLASS", "OVERRIDING_FINAL_MEMBER", "RETURN_TYPE_MISMATCH_ON_OVERRIDE", "CONFLICTING_OVERLOADS", "EXTERNAL_DELEGATION", "NESTED_CLASS_IN_EXTERNAL_INTERFACE")


This file seems to be missing a header comment

Thanks for pointing out!

pavlospt · 2018-05-03T20:09:30Z

@anton-bannykh Thank you for reviewing it, will get the fixes ASAP :)

pavlospt · 2018-05-03T20:22:05Z

@anton-bannykh Fixed those issues and checked the generated test code, that it properly includes the require('firebase-admin') & require('firebase-functions') declarations, respectively!

anton-bannykh · 2018-05-14T12:30:22Z

Hi! Sorry for the long wait once again =(

So I've been looking at the Firebase documentation. I have doubts it is possible to use the resulting JS code for the WEB use case: https://firebase.google.com/docs/web/setup

It seems the calls to Firebase API are wrong, when compiled with moduleKind=plain. Or am I wrong?

P.S. This might be not super-important for the first version. =)
P.P.S. I'm not really familiar with Firebase. Have been meaning to look into it, but things kept coming up =(

pavlospt · 2018-05-14T12:41:05Z

Hello @anton-bannykh, the Firebase converted files are for Node.JS SDK of Firebase Cloud Functions and Admin! Personally, I have tested handwritten Kotlin code and compiled to JS, which perfectly deploys to Cloud Functions! Not sure if the same SDKs are for Web usage.

pavlospt · 2018-06-16T15:18:12Z

Hey @anton-bannykh , it's been a month since the last fixes! Is there any chance that this will get merged? :)

anton-bannykh · 2018-06-25T09:24:28Z

@pavlospt Sorry, my bad! =( Will merge it asap.

pavlospt · 2018-07-18T13:08:29Z

Hey @anton-bannykh I am really sorry for pinging you again, but it has been 23 days since you said that it will be merged! I am completely sure that you probably have other things to do and forgot that, but I just wanted to see if you have some free time to merge this one :)

pavlospt force-pushed the firebase_externals branch 2 times, most recently from 3e92c48 to 78ff72e Compare April 5, 2018 08:04

Add Firebase externals for admin & functions

2acc286

pavlospt force-pushed the firebase_externals branch from 78ff72e to 2acc286 Compare April 5, 2018 08:05

bashor assigned anton-bannykh Apr 6, 2018

anton-bannykh suggested changes Apr 10, 2018

View reviewed changes

pavlospt added 6 commits April 10, 2018 20:59

Move firebase-admin & firebase-functions to externals

72fa6db

Update version folder names

7fd4da7

Add README.md files

7456758

Add packages

54848a1

Add copyright, license & contribution headers

6b868de

Add _testShared directories

800fe63

pavlospt added 2 commits April 11, 2018 14:50

Fix firebase-admin version

c28ec82

Rename version folder for firebase-admin

00318d6

anton-bannykh reviewed Apr 11, 2018

View reviewed changes

Fix export declare types that were missing

2308609

anton-bannykh suggested changes Apr 13, 2018

View reviewed changes

pavlospt added 2 commits April 13, 2018 23:16

Fix wrong package declaration

73a8698

Fix compilation errors

0f6bb42

pavlospt force-pushed the firebase_externals branch from 421566b to 0f6bb42 Compare April 13, 2018 21:33

anton-bannykh reviewed May 3, 2018

View reviewed changes

pavlospt added 4 commits May 3, 2018 23:12

Delete wrongly commited files

7cde54d

Add missing header comment

5daba94

Add missing annotations on firebase-admin files

5bb1032

Add missing annotations on firebase-functions files

699990d

pavlospt closed this Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Firebase externals for admin & functions #22

Add Firebase externals for admin & functions #22

pavlospt commented Apr 5, 2018 •

edited

bashor commented Apr 6, 2018

anton-bannykh left a comment •

edited

pavlospt commented Apr 10, 2018

pavlospt commented Apr 10, 2018

anton-bannykh commented Apr 11, 2018

pavlospt commented Apr 11, 2018

anton-bannykh commented Apr 11, 2018

anton-bannykh Apr 11, 2018

pavlospt Apr 11, 2018

anton-bannykh left a comment

anton-bannykh Apr 13, 2018

anton-bannykh Apr 13, 2018

pavlospt commented Apr 13, 2018 •

edited

pavlospt commented Apr 13, 2018 •

edited

pavlospt commented Apr 30, 2018

anton-bannykh commented Apr 30, 2018

pavlospt commented Apr 30, 2018

anton-bannykh commented May 3, 2018

anton-bannykh May 3, 2018

pavlospt May 3, 2018

pavlospt commented May 3, 2018

pavlospt commented May 3, 2018 •

edited

anton-bannykh commented May 14, 2018

pavlospt commented May 14, 2018

pavlospt commented Jun 16, 2018

anton-bannykh commented Jun 25, 2018

pavlospt commented Jul 18, 2018

		@@ -0,0 +1,182 @@
		@file:Suppress("INTERFACE_WITH_SUPERCLASS", "OVERRIDING_FINAL_MEMBER", "RETURN_TYPE_MISMATCH_ON_OVERRIDE", "CONFLICTING_OVERLOADS", "EXTERNAL_DELEGATION", "NESTED_CLASS_IN_EXTERNAL_INTERFACE")

Add Firebase externals for admin & functions #22

Add Firebase externals for admin & functions #22

Conversation

pavlospt commented Apr 5, 2018 • edited

bashor commented Apr 6, 2018

anton-bannykh left a comment • edited

Choose a reason for hiding this comment

pavlospt commented Apr 10, 2018

pavlospt commented Apr 10, 2018

anton-bannykh commented Apr 11, 2018

pavlospt commented Apr 11, 2018

anton-bannykh commented Apr 11, 2018

anton-bannykh Apr 11, 2018

Choose a reason for hiding this comment

pavlospt Apr 11, 2018

Choose a reason for hiding this comment

anton-bannykh left a comment

Choose a reason for hiding this comment

anton-bannykh Apr 13, 2018

Choose a reason for hiding this comment

anton-bannykh Apr 13, 2018

Choose a reason for hiding this comment

pavlospt commented Apr 13, 2018 • edited

pavlospt commented Apr 13, 2018 • edited

pavlospt commented Apr 30, 2018

anton-bannykh commented Apr 30, 2018

pavlospt commented Apr 30, 2018

anton-bannykh commented May 3, 2018

anton-bannykh May 3, 2018

Choose a reason for hiding this comment

pavlospt May 3, 2018

Choose a reason for hiding this comment

pavlospt commented May 3, 2018

pavlospt commented May 3, 2018 • edited

anton-bannykh commented May 14, 2018

pavlospt commented May 14, 2018

pavlospt commented Jun 16, 2018

anton-bannykh commented Jun 25, 2018

pavlospt commented Jul 18, 2018

pavlospt commented Apr 5, 2018 •

edited

anton-bannykh left a comment •

edited

pavlospt commented Apr 13, 2018 •

edited

pavlospt commented Apr 13, 2018 •

edited

pavlospt commented May 3, 2018 •

edited