Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: migrated link command to TS #696

Conversation

anikethsaha
Copy link
Contributor

Summary:

#683
Converted link command to Typescript
Also added preulink , postunlink to the cli-types in the config hooks

Test Plan:

  • yarn test
  • yarn lint

Copy link
Contributor

@FLGMwt FLGMwt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think this is one of the last of the commands. Just a few suggestions. 👐

packages/cli-types/src/index.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/link/__tests__/link-test.js Outdated Show resolved Hide resolved
packages/cli/src/commands/link/getPlatformName.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/link/linkAssets.ts Outdated Show resolved Hide resolved
@anikethsaha

This comment has been minimized.

@FLGMwt

This comment has been minimized.

@FLGMwt

This comment has been minimized.

@anikethsaha
Copy link
Contributor Author

I am getting a good number of errors when adding the type package for inquirer.

FlagsType.platforms (in link and unlink) from Array<'ios' | 'android'> to Array<'ios' | 'android'>

A bit confused with this one. Where do I get those ios and android type !

Looks like this worked out: 2eaeb70

Will rebase it and try it

@assuncaocharles
Copy link
Contributor

assuncaocharles commented Sep 11, 2019

You can repro this w/ yarn build:ts. Looks like inquirer has a types package so you can do:

yarn workspace @react-native-community/cli add @types/inquirer

....

Yes, I think it's missing the @types/inquirer and after installing it some things should change. e.g:

import {prompt, QuestionCollection, Answers} from 'inquirer';

export default (questions: QuestionCollection) =>
  new Promise<Answers>((resolve, reject) => {
    if (!questions) {
      resolve({});
      return;
    }

    prompt(questions).then(resolve, reject);
  });

And to fix the

Module '"readline"' has no exported member 'Interface'.
1 import { Interface as ReadlineInterface, Key } from "readline";

Just run yarn add @types/node

@anikethsaha

This comment has been minimized.

@anikethsaha

This comment has been minimized.

@assuncaocharles
Copy link
Contributor

assuncaocharles commented Sep 11, 2019

...
PS:
I even got both node and inquirer in my node_modules
....

That's weird, I reproduced it locally and it worked well.. Try to run yarn add @types/inquirer @types/node from inside packages/cli/src/commands

@anikethsaha
Copy link
Contributor Author

from packages/cli/src/commands or from packages/cli ? I guess from packages/cli/ right ?
I tried....no luck

@FLGMwt
Copy link
Contributor

FLGMwt commented Sep 11, 2019

Can you push you branch to so we can take a look? You should have @types/node in your packages/cli/package.json .

As for the export changes from my branch, if you're having issues with rebasing the changes, just copy over the changes to your branch. There should be changes in the following files:

  • packages/cli-types/src/index.ts
  • packages/platform-android/src/link/warnAboutManuallyLinkedLibs.ts
  • packages/platform-ios/src/link/warnAboutManuallyLinkedLibs.ts

@anikethsaha
Copy link
Contributor Author

@anikethsaha
Copy link
Contributor Author

As for the export changes from my branch, if you're having issues with rebasing the changes, just copy over the changes to your branch. There should be changes in the following files:

I manually copy pasted it......IDK why the rebasing didnt work !

@FLGMwt
Copy link
Contributor

FLGMwt commented Sep 11, 2019

I played around and I don't understand why, but adding an explicit version of @types/node made it work. I think it's something with how typescript resolved which type package to use for deps? It's probably the transitive @types/node@8 throwing things off but I have no idea. Give this a shot:

 yarn workspace @react-native-community/cli add @types/node@^12.7.5

Diff should look like:

image

@anikethsaha

This comment has been minimized.

@thymikee
Copy link
Member

We use @types/node@8 in root config to make sure we use APIs that are compatible with the target we compile to. Does inquirer has issues with older typings as well? cc @Esemesek halp pls

@anikethsaha
Copy link
Contributor Author

Does inquirer has issues with older typings as well?

I guess typescript is not able to resolve older versions.!

@FLGMwt
Copy link
Contributor

FLGMwt commented Sep 11, 2019

We use @types/node@8 in root config to make sure we use APIs that are compatible with the target we compile to. Does inquirer has issues with older typings as well? cc @Esemesek halp pls

Ah, that makes sense, sorry for the confusion. Seems like it used to be called ReadLine instead of Interface: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/v8/base.d.ts#L1850.

Latest types have Interface re-exported as ReadLine so maybe @types/inquirer could use that instead? https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/readline.d.ts#L110

Unsure what to do in cli though other than mark as untyped to get this in.

@anikethsaha
Copy link
Contributor Author

Should I downgrade the version then ?

gvarandas added a commit to gvarandas/cli that referenced this pull request Sep 11, 2019
@thymikee
Copy link
Member

thymikee commented Sep 12, 2019

To unblock ourselves I'm fine with not typing inquirer. We can do this later

thymikee pushed a commit to gvarandas/cli that referenced this pull request Sep 12, 2019
thymikee pushed a commit that referenced this pull request Sep 12, 2019
* chore: convert init command to TS

* Removing @types/inquirer package according to PR #696

* nits
@anikethsaha
Copy link
Contributor Author

Yeah this seems fine. I will remove typing for inquirer 👍

@anikethsaha

This comment has been minimized.

@thymikee

This comment has been minimized.

@anikethsaha

This comment has been minimized.

@thymikee

This comment has been minimized.

@anikethsaha

This comment has been minimized.

@thymikee

This comment has been minimized.

@anikethsaha

This comment has been minimized.

@thymikee

This comment has been minimized.

@anikethsaha

This comment has been minimized.

@anikethsaha anikethsaha changed the title chore: migrated link command to TS [WIP] chore: migrated link command to TS Sep 15, 2019
@anikethsaha

This comment has been minimized.

@thymikee thymikee force-pushed the migration/typescript-link-command branch from 552cab2 to f82c1cb Compare September 15, 2019 20:16
@thymikee thymikee changed the title [WIP] chore: migrated link command to TS chore: migrated link command to TS Sep 15, 2019
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Esemesek Esemesek merged commit 348a231 into react-native-community:master Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants