fix(types): enable TypeScript libCheck & resolve type conflicts #4306

davidgovea · 2020-09-25T06:57:00Z

Related issues

Fixes #2614
Also see #2732 , #3728

Description

I ❤️ skipLibCheck: false! The issues it catches are often non-trivial.

This PR changes the following:

Updates the TypeScript definition structure to allow skipLibCheck: false compatibility
- Avoids errors like:
  - Import declaration conflicts with local declaration of 'ReactNativeFirebase'
  - Exports and export assignments are not permitted in module augmentations.
  - Top-level declarations in .d.ts files must start with either a 'declare' or 'export' modifier.
Enabling the "libCheck" also uncovered some issues with the existing types, so these issues have been fixed with a minimal-change approach.
- For example, the SetCheckoutOptionEventParameters interface was missing, so it acted as any.
  - This PR changes the type to any, rather than adding the missing interface.
  - I will submit a separate PR with type improvements. Minimal-change approach here.

Approach:

Previously, module .d.ts files had the following:

declare module '@react-native-firebase/MODULE' with the module's exports
declare module '@react-native-firebase/app', augmenting the root firebase object
(Optional) a namespace ReactNativeFirebase to augment the FirebaseJsonConfig interface.

This PR switches (1) to root-level exports, leaves (2) as-is, and merges (3) into (2) to properly merge the namespace, rather than override it.

For the type fixes, each was placed into a separate commit for easier review. Can definitely squash if desired.. there are a handful!

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e [No code changes]
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- No

Test Plan

Type tests are passing, and one intentionally-incorrect test was updated with a type override, since TS now catches the error.

Test patches to be used with `patch-package` https://gist.github.com/davidgovea/2feea6af41b41f76fae1a9c14465cb69

Thanks for reading! I know that @mikehardy has probably already read this before I submitted it.. somehow.. 🧙

"dom" conflicts with @types/react-native

… than module declarations with namespace extensions

…declarations

…se` namespace from /app Re-exporting the namespace causes collision issues -- tramples previous values rather than merging

…eferring to interface keys

"Function" is not generic

Keep current behavior with `any`

Only allowed in implementations

…r `setBackgroundMessageHandler`

vercel · 2020-09-25T06:57:09Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/19f7umkno
✅ Preview: https://react-native-fireba-git-fork-davidgovea-fix-typescript-l-f7f980.invertase.vercel.app

CLAassistant · 2020-09-25T06:57:09Z

All committers have signed the CLA.

davidgovea · 2020-09-25T07:13:07Z

Here is a gist of each package's changes in patch files for use with patch-package

https://gist.github.com/davidgovea/2feea6af41b41f76fae1a9c14465cb69

davidgovea · 2020-09-25T15:33:52Z

@mikehardy It looks like the automatic patch creation is having issues: patches were only generated for app, analytics, and admob modules.

The others have errors like: Error: Can't find lockfile entry for @react-native-firebase/ml-vision

mikehardy · 2020-09-25T15:46:00Z

🤦 thanks for pointing that out - it's relatively new but I swear it was working when I tested it. Back to the workbench!

…arcodeDetectorTypes

…herited key Could have Omit<>'ed the incompatible `bounds` key, but duplicating the one inherited key seemed less complex

Incorrect type is now caught by TS

…ypes VisionBarcodeFormat.ALL_POINTS does not exist

davidgovea · 2020-09-25T15:56:18Z

Made an improvement to the ml-vision types -- I had previously dropped down to any on some of the MLKit types that were not found, but I noticed they existed in the BarcodeDetectorTypes.d.ts file, so I imported them. It also uncovered a typo in the type-test for ml-vision.

Updated the patches in my gist as well.

Salakar · 2020-09-28T06:43:52Z

Looks like theres an issue with typedocs command failing with these changes which is causing the vercel deploy preview failure:

Salakar · 2020-09-28T06:48:54Z

Side note we should probably run this script https://github.com/invertase/react-native-firebase/blob/master/website/scripts/generate-typedoc.js as part of the Code Quality Checks / TypeScript Build Validation (pull_request) CI stage

davidgovea · 2020-09-28T16:39:43Z

Thanks for reviewing @Salakar!

This is popping up because of the older version of TypeScript being used in this portion of the project (3.5.3) vs the root project's ^3.8.3.

TS improved recursive types a lot in 3.7 (see https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#more-recursive-type-aliases and microsoft/TypeScript#33050), and they make a lot of sense for the Firestore definitions, so it seems to me that upgrading typedoc is appropriate. I'll make an attempt.

Another TS feature that I wished was available for the type-tests is the // @ts-expect-error comment from 3.9. Would allow great improvements to the type testing in an easy way.

davidgovea · 2020-09-28T16:52:01Z

Well, that didn't go as planned.

Typedoc is currently locked to 0.15.0, 0.16 updates to TS >3.7, but it also changes some internal behavior. The inputFiles are empty when using a more recent version of typedoc (tried latest 0.19.2 and 0.16.0).

I'll be looking into it more when I have some more time to circle back.

mikehardy · 2020-09-28T17:34:41Z

Honest question: why not bump to typescript 4.x? I'm not aware of compatibility issues (I just bump typescript whenever it comes out since it has never caused me problems, and RNFB is fully working for me with typescript "current stable" (4.x whatever it is right now)

As a developer dependency that is just an optional support for the package I'd be +1 on bumping typescript all over. Historically I believe we have treated type breaks as a minor version ? if it is clearly documented we bumped minimum typescript version that would be okay with me but I'm also in the "version numbers are free" school of thought so could do semver major if needed (with same need for a clear breaking change / exact step to migrate snip in the changelog - easy to do)

davidgovea · 2020-09-28T17:39:22Z

Alrighty -- the website is building properly.

I don't have access to the vercel action info, but I have tested locally and things are looking good.

Updated to the latest version of Typedoc, and added the one required change: a new TSConfigReader reader plugin call.

@mikehardy -- I agree. I actually did bump TS to v4 in another side-branch, and things seemed fine. Just didn't want to overload this PR with extras.
Also, it looks like typedoc has not moved to v4 yet.

uses new syntax: TSConfigReader

mikehardy · 2020-09-30T19:31:55Z

packages/messaging/lib/index.d.ts

-    getToken(authorizedEntity?: string, scope?: string = 'FCM'): Promise<string>;
+    getToken(authorizedEntity?: string, scope?: string): Promise<string>;


Interesting - this default string value you are removing has some relation to this issue: #3714

Hey @mikehardy -- the place that the default scope = 'FCM' is set is in the JS code here: https://github.com/invertase/react-native-firebase/blob/master/packages/messaging/lib/index.js#L142-L159.

.d.ts definitions have no runtime responsibilities: parameter defaults are not allowed.

If changing the default scope is a change needed in #3714, it needs to happen in the source file.

Yes definitely - I just thought it was interesting the 'FCM' scoped was baked in here as well, and since it's an active discussion item I was cross-linking for cross-pollination. I commented over there that I'd love a PR to do the actual change

mikehardy

Just a note I scanned through these again they all seem reasonable. Going to test the patches now in my work project since otherwise the type checks here aren't very robust I don't think. I'm mostly interested in whether these cause any breaking changes or not, for correct callouts in release notes

mikehardy

I integrated all the patches in my work project and everything seems fine, no type breaks there. I'm not able to set skipLibCheck to false in my project due to other problems, but none of them were in react-native-firebase after this change

…rtase#4306) * chore: enable TypeScript libcheck & remove "dom" lib "dom" conflicts with @types/react-native * chore: add react types (referenced by admob types) * refactor(types, app): move "app" exports to top-level exports, rather than module declarations with namespace extensions * refactor(types): use top-level exports for module types, rather than declarations * refactor(types): for JSON config, augment existing `ReactNativeFirebase` namespace from /app Re-exporting the namespace causes collision issues -- tramples previous values rather than merging * fix(types, admob): Use string-indexing rather than dot-notation for referring to interface keys * fix(types, analytics): convert missing interface to `any` * fix(types, database): missing `value` type (treated as 'any') * fix(types, dynamic-links): fix dynamic-links `onLink` function type "Function" is not generic * fix(types, firestore): add missing generic to Promise type Keep current behavior with `any` * fix(types, messaging): remove parameter initializer in typedef Only allowed in implementations * fix(types, messaging): avoid implicit `any` via `void` return type for `setBackgroundMessageHandler` * fix(types, ml-vision): import non-ambient types in BarcodeDetectorTypes * fix(types, ml-vision): import non-ambient types in root module from BarcodeDetectorTypes * fix(types, ml-vision): repair invalid `extends` use by duplicating inherited key Could have Omit<>'ed the incompatible `bounds` key, but duplicating the one inherited key seemed less complex * fix(types, perf): add void return types to untyped perf methods * tests(admob): add type override to intentionally-wrong syntax Incorrect type is now caught by TS * tests(types, ml-vision): fix ml-vision test typo, exposed by proper types VisionBarcodeFormat.ALL_POINTS does not exist * chore(website): update typedoc to allow recursive types uses new syntax: TSConfigReader

It's not possible to untangle node and react-native types unfortunately, they clash. It is a known issue and apparently intractable, and new typescript projects have skipLibCheck on so we acquiesce Related #4306

…rtase#4306) * chore: enable TypeScript libcheck & remove "dom" lib "dom" conflicts with @types/react-native * chore: add react types (referenced by admob types) * refactor(types, app): move "app" exports to top-level exports, rather than module declarations with namespace extensions * refactor(types): use top-level exports for module types, rather than declarations * refactor(types): for JSON config, augment existing `ReactNativeFirebase` namespace from /app Re-exporting the namespace causes collision issues -- tramples previous values rather than merging * fix(types, admob): Use string-indexing rather than dot-notation for referring to interface keys * fix(types, analytics): convert missing interface to `any` * fix(types, database): missing `value` type (treated as 'any') * fix(types, dynamic-links): fix dynamic-links `onLink` function type "Function" is not generic * fix(types, firestore): add missing generic to Promise type Keep current behavior with `any` * fix(types, messaging): remove parameter initializer in typedef Only allowed in implementations * fix(types, messaging): avoid implicit `any` via `void` return type for `setBackgroundMessageHandler` * fix(types, ml-vision): import non-ambient types in BarcodeDetectorTypes * fix(types, ml-vision): import non-ambient types in root module from BarcodeDetectorTypes * fix(types, ml-vision): repair invalid `extends` use by duplicating inherited key Could have Omit<>'ed the incompatible `bounds` key, but duplicating the one inherited key seemed less complex * fix(types, perf): add void return types to untyped perf methods * tests(admob): add type override to intentionally-wrong syntax Incorrect type is now caught by TS * tests(types, ml-vision): fix ml-vision test typo, exposed by proper types VisionBarcodeFormat.ALL_POINTS does not exist * chore(website): update typedoc to allow recursive types uses new syntax: TSConfigReader

davidgovea added 13 commits September 24, 2020 17:37

chore: enable TypeScript libcheck & remove "dom" lib

156a557

"dom" conflicts with @types/react-native

chore: add react types (referenced by admob types)

4deded3

refactor(types, app): move "app" exports to top-level exports, rather…

e1eae60

… than module declarations with namespace extensions

refactor(types): use top-level exports for module types, rather than …

a921b56

…declarations

refactor(types): for JSON config, augment existing `ReactNativeFireba…

bf9dd43

…se` namespace from /app Re-exporting the namespace causes collision issues -- tramples previous values rather than merging

fix(types, admob): Use string-indexing rather than dot-notation for r…

3443801

…eferring to interface keys

fix(types, analytics): convert missing interface to any

0ddff12

fix(types, database): missing value type (treated as 'any')

af23dbb

fix(types, dynamic-links): fix dynamic-links onLink function type

b85c6b4

"Function" is not generic

fix(types, firestore): add missing generic to Promise type

0aeddbe

Keep current behavior with `any`

fix(types, messaging): remove parameter initializer in typedef

41fc035

Only allowed in implementations

fix(types, messaging): avoid implicit any via void return type fo…

f6ef17f

…r `setBackgroundMessageHandler`

fix(types, ml-vision): import non-ambient types in BarcodeDetectorTypes

193bc33

vercel bot had a problem deploying to Preview September 25, 2020 06:57 Failure

davidgovea mentioned this pull request Sep 25, 2020

🔥 TypeScript errors #2614

Closed

10 tasks

davidgovea changed the title ~~TypeScript Refactor: enable libCheck~~ fix(types): enable TypeScript libCheck & resolve type conflicts Sep 25, 2020

davidgovea force-pushed the fix/typescript-libcheck branch from 6c17ed0 to 0ec9750 Compare September 25, 2020 07:23

vercel bot had a problem deploying to Preview September 25, 2020 07:23 Failure

mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. tools: typings TypeScript / Flow Type: Bug Fix labels Sep 25, 2020

davidgovea added 4 commits September 25, 2020 08:49

fix(types, ml-vision): import non-ambient types in root module from B…

a099110

…arcodeDetectorTypes

fix(types, ml-vision): repair invalid extends use by duplicating in…

bf5f36c

…herited key Could have Omit<>'ed the incompatible `bounds` key, but duplicating the one inherited key seemed less complex

fix(types, perf): add void return types to untyped perf methods

03ddd88

tests(admob): add type override to intentionally-wrong syntax

d5644c5

Incorrect type is now caught by TS

tests(types, ml-vision): fix ml-vision test typo, exposed by proper t…

c2e8023

…ypes VisionBarcodeFormat.ALL_POINTS does not exist

davidgovea force-pushed the fix/typescript-libcheck branch from 0ec9750 to c2e8023 Compare September 25, 2020 15:51

vercel bot had a problem deploying to Preview September 25, 2020 15:51 Failure

mikehardy mentioned this pull request Sep 28, 2020

Run generate-typedoc.js in CI code quality checks #4321

Closed

vercel bot deployed to Preview September 28, 2020 17:24 View deployment

chore(website): update typedoc to allow recursive types

176fb12

uses new syntax: TSConfigReader

davidgovea force-pushed the fix/typescript-libcheck branch from 15d0751 to 176fb12 Compare September 28, 2020 17:55

vercel bot deployed to Preview September 28, 2020 17:56 View deployment

mikehardy mentioned this pull request Sep 28, 2020

[🐛] NPM package '@react-native-firebase/messaging' depends on '@react-native-firebase/app' v8.4.2 but found v8.4.3 #4283

Closed

mikehardy reviewed Sep 30, 2020

View reviewed changes

mikehardy approved these changes Sep 30, 2020

View reviewed changes

mikehardy merged commit aa8ee8b into invertase:master Sep 30, 2020

mikehardy mentioned this pull request Sep 30, 2020

CI patch-package task failing to generate full set of patches #4331

Closed

davidgovea mentioned this pull request Oct 1, 2020

chore(ci): run TypeDoc generation in Code Quality Checks action #4345

Merged

8 tasks

mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Dec 2, 2020

antoinewg mentioned this pull request Dec 8, 2020

fix(docs): correct path to source code #4659

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(types): enable TypeScript libCheck & resolve type conflicts #4306

fix(types): enable TypeScript libCheck & resolve type conflicts #4306

davidgovea commented Sep 25, 2020 •

edited

Loading

vercel bot commented Sep 25, 2020 •

edited

Loading

CLAassistant commented Sep 25, 2020 •

edited

Loading

davidgovea commented Sep 25, 2020

davidgovea commented Sep 25, 2020

mikehardy commented Sep 25, 2020

davidgovea commented Sep 25, 2020

Salakar commented Sep 28, 2020

Salakar commented Sep 28, 2020

davidgovea commented Sep 28, 2020

davidgovea commented Sep 28, 2020

mikehardy commented Sep 28, 2020

davidgovea commented Sep 28, 2020

mikehardy Sep 30, 2020

davidgovea Sep 30, 2020

mikehardy Sep 30, 2020

mikehardy left a comment

mikehardy left a comment

		getToken(authorizedEntity?: string, scope?: string = 'FCM'): Promise<string>;
		getToken(authorizedEntity?: string, scope?: string): Promise<string>;

fix(types): enable TypeScript libCheck & resolve type conflicts #4306

fix(types): enable TypeScript libCheck & resolve type conflicts #4306

Conversation

davidgovea commented Sep 25, 2020 • edited Loading

Related issues

Description

Approach:

Checklist

Test Plan

Test patches to be used with patch-package https://gist.github.com/davidgovea/2feea6af41b41f76fae1a9c14465cb69

vercel bot commented Sep 25, 2020 • edited Loading

CLAassistant commented Sep 25, 2020 • edited Loading

davidgovea commented Sep 25, 2020

davidgovea commented Sep 25, 2020

mikehardy commented Sep 25, 2020

davidgovea commented Sep 25, 2020

Salakar commented Sep 28, 2020

Salakar commented Sep 28, 2020

davidgovea commented Sep 28, 2020

davidgovea commented Sep 28, 2020

mikehardy commented Sep 28, 2020

davidgovea commented Sep 28, 2020

mikehardy Sep 30, 2020

Choose a reason for hiding this comment

davidgovea Sep 30, 2020

Choose a reason for hiding this comment

mikehardy Sep 30, 2020

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

davidgovea commented Sep 25, 2020 •

edited

Loading

Test patches to be used with `patch-package` https://gist.github.com/davidgovea/2feea6af41b41f76fae1a9c14465cb69

vercel bot commented Sep 25, 2020 •

edited

Loading

CLAassistant commented Sep 25, 2020 •

edited

Loading