Add typescript declaration files for all public APIs #247

janpieterz · 2018-02-23T07:28:16Z

Let me know if this is appreciated, I can add types for the other packages too.

msftclas · 2018-02-23T07:28:27Z

All CLA requirements met.

guperrot · 2018-02-23T20:07:20Z

(Subset of #172)

appcenter-analytics/Analytics.d.ts

elamalani · 2018-02-26T23:16:43Z

@janpieterz Thanks for submitting a PR in our SDK and we really appreciate your contribution. Our team reviewed your PR and it looks like you are adding typescript definition only in Analytics module. For this to be consistent across, all the modules - appcenter, appcenter-push, appcenter-crashes should have the definitions as well.

guperrot · 2018-02-26T23:21:43Z

We would also need to make a Typescript version of TestApp before merging to validate the files.

janpieterz · 2018-03-01T08:05:46Z

I'll tackle the appcenter, appcenter-push, appcenter-crashes as well, and create a typescript version of the TestApp. Might take a bit.

Would you prefer me tackling those all at once or pushing the definitions out per package so we keep the stuff to review small?

guperrot · 2018-03-03T00:50:50Z

If we merge 1 by 1 then the target branch cannot be develop and we can create another hosting branch. Github offers incremental reviews since last time so it's not a problem for me as long as commits for TestApp are separated from the SDK changes it should be ok.

guperrot · 2018-03-03T00:51:33Z

Changes on analytics are ok so far.

janpieterz · 2018-03-12T10:53:37Z

Should have all typings now, let me know what's wrong and I'll fix it, then move on to the TestApp.

appcenter-crashes/Crashes.d.ts

appcenter-push/Push.d.ts

appcenter/AppCenter.d.ts

guperrot

We would need a test app in same pr to validate the changes. Minor comments otherwise.
Nice progress on this 👍

appcenter-crashes/Crashes.d.ts

guperrot · 2018-03-20T17:50:03Z

Looks good, now we would need a clone of the testapp we have in the repo but in typescript version to validate all the changes.

janpieterz · 2018-03-20T17:51:23Z

Great stuff, will start tackling that.

janpieterz · 2018-04-04T12:54:01Z

@guperrot Made some progress with the test app.

Couple of questions:

PushNotification title can currently be a string, null or undefined. It looks like the test app doesn't account for it possibly being undefined. Is this something to change in the Typescript definition? Is it only null/undefined if message is null/undefined?
ErrorAttachmentLog.attachmentWithBinary seems to be called with a string from the test apps, is this always the case?
Did I miss shouldProcess and shouldAwaitUserConfirmation in the CrashesListener interface before? I've added them now, just making sure it's not a private API and it's supposed to be public.
On analyticsScreen a variable data is used, undefined (I think). It's for Track Event badly (Do not do this, only strings are supported). What is supposed to happen here? It currently doesn't do anything.

reset my feedback to review new changes

TestAppTypescript/android/app/src/main/java/com/testapptypescript/MainApplication.java

TestAppTypescript/android/app/src/main/res/values/strings.xml

TestAppTypescript/ios/TestAppTypescript/AppDelegate.m

TestAppTypescript/package.json

guperrot · 2018-04-06T00:10:08Z

Replies to #247 (comment)

Sounds like a bug, it should be string or null but not undefined. We will have to investigate.
From Javascript we pass binary as string with base64 encoding, only native SDK APIs have support for byte arrays.
Those APIs are not new and are publicly documented.
I have to check but AppCenter is supposed to log an error in the console and not send the event. In TypeScript we have types so you don't need that button I guess.

I made a first quick review for changes on TestApp and need some fixes before I can deeply review and test.

guperrot

Copying comment as github collasped it and link does not work: #247 (comment)

Like in TestApp, make sure firebase is integrated: https://docs.microsoft.com/en-us/appcenter/sdk/push/migration/react-native-android.

When the procedure asks to download google-services.json, pick https://github.com/Microsoft/AppCenter-SDK-React-Native/blob/develop/TestApp/android/app/google-services.json instead and make sure your app has the same package name as TestApp.

janpieterz · 2018-09-12T07:15:06Z

@guperrot Google Services added, not sure what else there is atm.

guperrot

No matter how I git clean -xdf and npm cache clean --force, when I checkout this branch, run ./update-npm-package.sh and react-native run-android, the JS server fails to bundle and thus the app is broken:

error: bundling failed: Error: Unable to resolve module `./artifacts/index` from `/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/index.js`: The module `./artifacts/index` could not be found from `/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/index.js`. Indeed, none of these files exist:

  * `/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/artifacts/index(.native||.android.js|.native.js|.js|.android.json|.native.json|.json)`
  * `/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/artifacts/index/index(.native||.android.js|.native.js|.js|.android.json|.native.json|.json)`
    at ModuleResolver.resolveDependency (/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/node-haste/DependencyGraph/ModuleResolution.js:158:865)
    at ResolutionRequest.resolveDependency (/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/node-haste/DependencyGraph/ResolutionRequest.js:92:16)
    at DependencyGraph.resolveDependency (/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/node-haste/DependencyGraph.js:271:4465)
    at dependencies.map.relativePath (/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/DeltaBundler/traverseDependencies.js:379:19)
    at Array.map (<anonymous>)
    at resolveDependencies (/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/DeltaBundler/traverseDependencies.js:378:16)
    at /Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/DeltaBundler/traverseDependencies.js:203:33
    at Generator.next (<anonymous>)
    at step (/Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/DeltaBundler/traverseDependencies.js:296:307)
    at /Users/guperrot/git/appcenter-sdk-react-native/TestAppTypescript/node_modules/metro/src/DeltaBundler/traverseDependencies.js:296:467
 BUNDLE  [android, dev] ./index.js ░░░░░░░░░░░░░░░░ 0.0% (0/1), failed.

Please look into this so we can review this PR, none of the search results on Google were helpful for this issue.

The same problem exists for ios.

FutureGUIs · 2018-09-12T22:02:32Z

I'm not sure it's truly the fix for that latest 'bundling' error, but I used to get it when just running the normal react-native android or ios on RN 0.54 or 0.55. But then I would run a separate npm start or the RN \cli.js start and I'd be able to run my app. And after upgrading to the new RN 0.57 release, that issue has gone away.

guperrot · 2018-09-12T22:09:06Z

I already tried npm run start as separate but it didn't change anything, same error.

janpieterz · 2018-09-13T08:58:20Z

@guperrot This is because you're not running the Typescript compiler first. You're welcome to change the commands to something more to your liking, running npm run watch is needed.

There should be commands for npm run ios and npm run android that should automatically start the correct watcher and react-native run-X.

guperrot · 2018-09-13T21:46:56Z

npm run watch did the trick, I could run test app successfully on both platforms.
However I am investigating on typing inconsistencies in crash report, will elaborate soon but we might have to update either SDK or typings.

guperrot

Tested everything and found some problems, nice job otherwise and looking forward to merging soon after the requested changes 👍

Please also copy .vscode/launch.json from TestApp to this app and commit it.

appcenter-crashes/Crashes.d.ts

appcenter-push/Push.d.ts

appcenter/AppCenter.d.ts

TestAppTypescript/ios/TestAppTypescript/Info.plist

TestAppTypescript/ios/TestAppTypescript.xcodeproj/project.pbxproj

guperrot

Add typescript declaration files for all public APIs #247 (comment) for Push.
Please also copy .vscode/launch.json from TestApp to this app and commit it.
Add typescript declaration files for all public APIs #247 (comment)
Add typescript declaration files for all public APIs #247 (review)

appcenter-analytics/Analytics.d.ts

janpieterz · 2018-09-25T11:08:48Z

@guperrot Should all be fixed.

guperrot · 2018-09-25T21:37:57Z

@janpieterz thanks for this huge contribution 👍

dhei · 2018-09-25T21:40:14Z

Hey @janpieterz,

I want to say a big thank you! Your contribution is highly appreciated. 👍

Thanks,
Di

janpieterz · 2018-09-26T08:15:01Z

Thanks for the help gents, happy we landed it :)

janpieterz added 2 commits February 23, 2018 08:11

Add analytics typescript definitions

324b736

Fix prettier line rewrites

b4c6148

guperrot added the feature request label Feb 23, 2018

guperrot suggested changes Feb 26, 2018

View reviewed changes

appcenter-analytics/Analytics.d.ts Outdated Show resolved Hide resolved

appcenter-analytics/Analytics.d.ts Outdated Show resolved Hide resolved

appcenter-analytics/Analytics.d.ts Outdated Show resolved Hide resolved

Adjust typescript Analytics definitions

4ba86f0

janpieterz added 4 commits March 12, 2018 11:34

Add Crashes definitions

39287b2

Return promise at Crashes setListener

a923928

Add push typings

100b5cb

Add AppCenter types

6773f68

guperrot suggested changes Mar 12, 2018

View reviewed changes

Fix typos, missing items etc

ddd577f

guperrot previously requested changes Mar 19, 2018

View reviewed changes

appcenter-crashes/Crashes.d.ts Outdated Show resolved Hide resolved

appcenter-crashes/Crashes.d.ts Outdated Show resolved Hide resolved

Rename ListenerMap to CrashesListener

077d3a9

guperrot changed the title ~~Add types for appcenter-analytics~~ Add typescript declaration files for all public APIs Mar 20, 2018

janpieterz added 2 commits April 3, 2018 09:40

Initial working Typescript TestApp

52f3b04

Add test app and change types accordingly

59cf689

guperrot suggested changes Apr 6, 2018

View reviewed changes

Remove upper level package-lock

3dfb3a7

guperrot suggested changes Sep 6, 2018

View reviewed changes

janpieterz and others added 5 commits September 11, 2018 11:54

Add firebase

7b7cb7f

Merge remote-tracking branch 'upstream/develop' into develop

5ceba77

Set gradle to 4.4

a869d2d

TestApp is registered component instead of TestAppTypescript

d3bdb37

iOS start correct bundle

bcc2b6b

guperrot suggested changes Sep 12, 2018

View reviewed changes

guperrot suggested changes Sep 13, 2018

View reviewed changes

janpieterz added 3 commits September 19, 2018 08:30

Crashes typing updates

e92ad87

Update Typescript definitions

be1f292

Apply project diff

674b15a

guperrot suggested changes Sep 19, 2018

View reviewed changes

appcenter-analytics/Analytics.d.ts Outdated Show resolved Hide resolved

guperrot suggested changes Sep 19, 2018

View reviewed changes

appcenter-analytics/Analytics.d.ts Outdated Show resolved Hide resolved

janpieterz added 3 commits September 25, 2018 11:11

IUp definitions

d3423d1

Add launch.json

f8a38e3

Add push and background notification

f80b264

dhei approved these changes Sep 25, 2018

View reviewed changes

guperrot approved these changes Sep 25, 2018

View reviewed changes

guperrot merged commit d1ed9fc into microsoft:develop Sep 25, 2018

guperrot added the community contribution label Jan 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typescript declaration files for all public APIs #247

Add typescript declaration files for all public APIs #247

janpieterz commented Feb 23, 2018

msftclas commented Feb 23, 2018 •

edited

Loading

guperrot commented Feb 23, 2018

elamalani commented Feb 26, 2018

guperrot commented Feb 26, 2018

janpieterz commented Mar 1, 2018

guperrot commented Mar 3, 2018

guperrot commented Mar 3, 2018

janpieterz commented Mar 12, 2018

guperrot left a comment •

edited

Loading

guperrot commented Mar 20, 2018

janpieterz commented Mar 20, 2018

janpieterz commented Apr 4, 2018

guperrot commented Apr 6, 2018 •

edited

Loading

guperrot left a comment •

edited

Loading

janpieterz commented Sep 12, 2018

guperrot left a comment •

edited

Loading

FutureGUIs commented Sep 12, 2018

guperrot commented Sep 12, 2018

janpieterz commented Sep 13, 2018

guperrot commented Sep 13, 2018

guperrot left a comment •

edited

Loading

guperrot left a comment •

edited

Loading

janpieterz commented Sep 25, 2018

guperrot commented Sep 25, 2018

dhei commented Sep 25, 2018

janpieterz commented Sep 26, 2018

Add typescript declaration files for all public APIs #247

Add typescript declaration files for all public APIs #247

Conversation

janpieterz commented Feb 23, 2018

msftclas commented Feb 23, 2018 • edited Loading

guperrot commented Feb 23, 2018

elamalani commented Feb 26, 2018

guperrot commented Feb 26, 2018

janpieterz commented Mar 1, 2018

guperrot commented Mar 3, 2018

guperrot commented Mar 3, 2018

janpieterz commented Mar 12, 2018

guperrot left a comment • edited Loading

Choose a reason for hiding this comment

guperrot commented Mar 20, 2018

janpieterz commented Mar 20, 2018

janpieterz commented Apr 4, 2018

guperrot commented Apr 6, 2018 • edited Loading

guperrot left a comment • edited Loading

Choose a reason for hiding this comment

janpieterz commented Sep 12, 2018

guperrot left a comment • edited Loading

Choose a reason for hiding this comment

FutureGUIs commented Sep 12, 2018

guperrot commented Sep 12, 2018

janpieterz commented Sep 13, 2018

guperrot commented Sep 13, 2018

guperrot left a comment • edited Loading

Choose a reason for hiding this comment

guperrot left a comment • edited Loading

Choose a reason for hiding this comment

janpieterz commented Sep 25, 2018

guperrot commented Sep 25, 2018

dhei commented Sep 25, 2018

janpieterz commented Sep 26, 2018

msftclas commented Feb 23, 2018 •

edited

Loading

guperrot left a comment •

edited

Loading

guperrot commented Apr 6, 2018 •

edited

Loading

guperrot left a comment •

edited

Loading

guperrot left a comment •

edited

Loading

guperrot left a comment •

edited

Loading

guperrot left a comment •

edited

Loading