-
Couldn't load subscription status.
- Fork 12
Add Firebase externals for admin & functions #22
Conversation
3e92c48 to
78ff72e
Compare
78ff72e to
2acc286
Compare
Thank you for the contribution! We'll review it as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.ztheversioninkonfig.jsonshould bex.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.mdfiles. 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. =)
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 :)
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?
@pavlospt Thanks! Regarding 3) answered in slack. We probably should add that to the README.
- Could be. Was it
@firebase/functions-typesyou were converting?
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!
Well, it's no exactly a blocker. It should be possible to be fixed by translating that bit by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't compile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix that and other occurrences like it by hand!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ doesn't compile =(
It should have been just AppOptions, coupled with an import js.externals.firebase.admin.AppOptions at the beginning of the file.
@anton-bannykh perfect, thanks for checking it, will resolve the issues and get back :)
@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!
421566b to
0f6bb42
Compare
* 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)
Hey, kind re-ping on this :) Has anyone had the time to take a look :) ?
@pavlospt Sorry, I've been on vacation. =( Will be back to work on Wednesday. Sorry for letting you hanging for so long =(
@anton-bannykh Ohh sorry did not know! Please take your time :)
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to be missing a header comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out!
@anton-bannykh Thank you for reviewing it, will get the fixes ASAP :)
@anton-bannykh Fixed those issues and checked the generated test code, that it properly includes the require('firebase-admin') & require('firebase-functions') declarations, respectively!
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 =(
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.
Hey @anton-bannykh , it's been a month since the last fixes! Is there any chance that this will get merged? :)
@pavlospt Sorry, my bad! =( Will merge it asap.
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 :)
Uh oh!
There was an error while loading. Please reload this page.
Since this is my first contribution, please let me know if I miss anything or something is wrong :)