refactor: Migrate to typescript #95

g-dimitry · 2021-09-12T09:59:30Z

Hello Guys,

I've successfully migrated all the code base to Typescript. My philosophy was to try as much as I can to make this a minor release (as @mikehardy stated). You'll see I have added three TODO comments (search for "TODO: [TS Migration]:"). Those are changes that need to be discussed and addressed maybe in the next major release.

To test the compatibility of both the new types and the old types:

I reverted the commit removing the old types
Created an index.ts file at the root of the repo
Imported both new and old types the same way so: import * as oldTypes from 'react-native-fbsdk-next' and import * as newTypes from './src'
Created a new constant with the same type as the oldTypes' import and assign to it the newTypes: const o: typeof oldTypes = newTypes;
Solved all differences until the compiler had no errors. (All changes are marked by a "TODO: [TS Migration]:" comment)

I guess we need to test it on a running project first to check it works with a JS project & a TS project in development and release modes.

If anybody can test the above and confirm it's not breaking it would be awesome.

Looking forward to your comments.

g-dimitry · 2021-09-12T10:02:49Z

And of course the eslint/prettier configurations should be changed.

g-dimitry · 2021-09-12T12:43:12Z

I just changed the Eslint configuration but I had to change A LOT of code that may break old code so I guess the best way to do this is to agree on what to do with this migration: should we release a minor release and then a new major release with the linted and clean code (that may break some old false types) ?

mikehardy · 2021-09-13T03:39:42Z

this sounds amazing!
with sincere apologies I'm traveling until the 21st and then will likely have a backlog of project work to slash through before I can spin through volunteer repos so perhaps someone else can give this a spin 🙏 ? Otherwise it'll have to sit for a little bit ...

mikehardy · 2021-09-13T03:41:04Z

(I can say philosophically just in my opinion I think a major is probably warranted, the goal was just for it to be easiest possible migration for any module users, as a general courtesy - I'm guessing it's probably walking that fine line well just based on how you're describing it though I admit I haven't looked)

thebergamo · 2021-09-14T16:55:10Z

I was considering picking up this task together with upgrading Facebook SDK.
Can't thank you more @g-dimitry for pick this challenge of migration to TS.
Let me review it o/

thebergamo

There are few questions of mine also inside some points, specially this name: string | undefined as I believe we should stick to a pattern in this case using name?: string as in most cases would have similar effect.

Once again thanks for this PR o/

src/FBAccessToken.ts

thebergamo · 2021-09-14T17:02:01Z

src/FBAppEventsLogger.ts

@@ -131,7 +133,7 @@ module.exports = {
  logEvent(eventName: string, ...args: Array<number | Params>) {
    let valueToSum = 0;
    if (typeof args[0] === 'number') {
-      valueToSum = args.shift();
+      valueToSum = args.shift() as number;


Maybe instead of using ’as’ here, you could use ’Number(args.shift()’
What do you think?

thebergamo · 2021-09-14T17:07:19Z

src/FBGraphRequestManager.ts

+    // TODO: [TS Migration]: Added default value to fix type definitions. Discuss before merging.
+    this.batchCallback = () => {};


Isn't the case here to just use this.batchCallback?.() and then you won't need to put a default value here?
But either way having a default noop here seems fine to me.

thebergamo · 2021-09-14T17:08:14Z

src/FBLoginButton.tsx

  DefaultAudience,
  LoginBehaviorAndroid,
  LoginResult,
  LoginTracking,
 } from './FBLoginManager';

-type Event = Object;
-type TooltipBehaviorIOS = 'auto' | 'force_display' | 'disable';
+export type Event = any;


Can't we be a bit more strict here?

thebergamo · 2021-09-14T17:10:14Z

src/FBProfile.ts

@@ -73,17 +70,18 @@ class FBProfile {
    this.linkURL = profileMap.linkURL;
    this.imageURL = profileMap.imageURL;
    this.userID = profileMap.userID;
-    this.email = Platform.OS === 'android' ? null : profileMap.email;
+    // TODO: [TS Migration]: Value should be undefined. Discuss before merging.
+    this.email = Platform.OS === 'android' ? null as unknown as undefined : profileMap.email;


In this case we could just mark email: string? | null and we can avoid this unknown type

src/index.ts

g-dimitry · 2021-09-16T17:55:12Z

@thebergamo I'll finalize this on Saturday and I'll be sure to specify all breaking changes and tag you to discuss before merging. Thank you for the collaboration invitation, I'm very excited to help 😄

thebergamo · 2021-09-30T04:31:28Z

@g-dimitry i guess you would need to rebase as some new feature is added :)

g-dimitry · 2021-09-30T12:16:31Z

@thebergamo Unfortunately yes, Sorry for being late, Fell horribly ill. I guess I'll merge master into my branch it would be easier as I have 18 commits which will be a nightmare to rebase.

thebergamo · 2021-10-03T12:58:41Z

That's ok mate, your health is much more important o/ get great recovery 😃

stale · 2021-12-14T08:56:27Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rnike · 2021-12-16T14:42:20Z

Just saw this before making the new implementation, does this need help? Maybe I can help complete this before implementing the new feature.

mikehardy · 2021-12-16T14:59:46Z

@mike this one really needs help - yes! It would be fantastic to have this be in typescript and I don't believe the library has moved on that far since it was initially begun here

rnike · 2021-12-16T15:10:51Z

okay let me take a look at it

rnike · 2021-12-29T15:30:50Z

Commits in this pr have been merged in #160, thanks for all the works.

tet.bot added 17 commits September 12, 2021 09:36

Remove old type definitions

3a7ab3c

Init typescript

6e22af7

Migrate source to typescript

4b19191

Remove use strict

f092c57

Replace 'import type' with 'import'

114e17a

Export all types

b5058c7

Lint source

c3c6f05

Unify array type syntax

5d5ceaf

Fix tsconfig

96adca4

Remove unnecessary change

f9f3917

Change react-native value in package.json

c9d8707

Remove unnecessary changes

4cf1ca2

Fix index

9ba296b

Hot fix breaking changes and add TODOs

0b63055

Fix index exports

475aa78

Add index todo

36ba1c1

Lint source

5aa61a9

g-dimitry changed the title ~~Migrate to typescript~~ refactor: Migrate to typescript Sep 12, 2021

thebergamo requested changes Sep 14, 2021

View reviewed changes

mikehardy mentioned this pull request Sep 29, 2021

fix(type): add new testID to typescript definition file #103

Merged

Update Eslint and lint source

0ac2bd9

Sync with master

bf664ae

Typescript improvements

16a4e3a

This was referenced Nov 15, 2021

Open letter - Road to 7.0.0 #16

Closed

‘AccessToken’ refers to a value, but is being used as a type here. Did you mean ‘typeof AccessToken?’ #92

Closed

stale bot added the wontfix This will not be worked on label Dec 14, 2021

stale bot removed the wontfix This will not be worked on label Dec 16, 2021

rnike mentioned this pull request Dec 16, 2021

refactor!: migrate to typescript #155

Merged

rnike closed this Dec 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Migrate to typescript #95

refactor: Migrate to typescript #95

g-dimitry commented Sep 12, 2021

g-dimitry commented Sep 12, 2021

g-dimitry commented Sep 12, 2021

mikehardy commented Sep 13, 2021

mikehardy commented Sep 13, 2021

thebergamo commented Sep 14, 2021

thebergamo left a comment

thebergamo Sep 14, 2021

thebergamo Sep 14, 2021

thebergamo Sep 14, 2021

thebergamo Sep 14, 2021

g-dimitry commented Sep 16, 2021

thebergamo commented Sep 30, 2021

g-dimitry commented Sep 30, 2021

thebergamo commented Oct 3, 2021

stale bot commented Dec 14, 2021

rnike commented Dec 16, 2021

mikehardy commented Dec 16, 2021

rnike commented Dec 16, 2021

rnike commented Dec 29, 2021

		// TODO: [TS Migration]: Added default value to fix type definitions. Discuss before merging.
		this.batchCallback = () => {};

refactor: Migrate to typescript #95

refactor: Migrate to typescript #95

Conversation

g-dimitry commented Sep 12, 2021

g-dimitry commented Sep 12, 2021

g-dimitry commented Sep 12, 2021

mikehardy commented Sep 13, 2021

mikehardy commented Sep 13, 2021

thebergamo commented Sep 14, 2021

thebergamo left a comment

Choose a reason for hiding this comment

thebergamo Sep 14, 2021

Choose a reason for hiding this comment

thebergamo Sep 14, 2021

Choose a reason for hiding this comment

thebergamo Sep 14, 2021

Choose a reason for hiding this comment

thebergamo Sep 14, 2021

Choose a reason for hiding this comment

g-dimitry commented Sep 16, 2021

thebergamo commented Sep 30, 2021

g-dimitry commented Sep 30, 2021

thebergamo commented Oct 3, 2021

stale bot commented Dec 14, 2021

rnike commented Dec 16, 2021

mikehardy commented Dec 16, 2021

rnike commented Dec 16, 2021

rnike commented Dec 29, 2021