-
Notifications
You must be signed in to change notification settings - Fork 931
chore: migrate config command to TS #689
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: migrate config command to TS #689
Conversation
| import {Config} from '@react-native-community/cli-types'; | ||
|
|
||
| function isValidRNDependency(config) { | ||
| function isValidRNDependency(config: any) { |
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.
I think this config can be typed as:
config: AndroidDependencyConfig | IOSDependencyConfig | nullThere 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.
I'm having trouble to set the type of this parameter, I've tried this approach but it isn't working
| name: 'config', | ||
| description: 'Print CLI configuration', | ||
| func: async (argv: string[], ctx: ConfigT) => { | ||
| func: async (ctx: Config) => { |
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.
What happened to the first argument of the function? Have you made sure everything works as expected with this change?
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.
Config is a second argument. This change breaks that. If argv is not used, prepend it with an underscore: _argv
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.
OK. I din't notice that, I'll fix it.
thymikee
left a 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.
Hey, thanks for the PR! Please implement the feedback from @Esemesek and rebase to latest master to resolve conflicts :)
a454ad8 to
10eb772
Compare
|
Looks like TS is failing |
|
|
||
| function isValidRNDependency(config) { | ||
| function isValidRNDependency( | ||
| config: AndroidDependencyConfig | IOSDependencyConfig | null, |
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.
Just an idea, but maybe we want to the Dependency in @react-native-community/cli-types as an interface like:
export interface Dependency {
[key: string]: {
name: string;
root: string;
platforms: {
android?: AndroidDependencyConfig | null;
ios?: IOSDependencyConfig | null;
[key: string]: any;
};
assets: string[];
hooks: {
prelink?: string;
postlink?: string;
};
params: InquirerPrompt[];
};
}Don't forgetting to update the Config interface to dependencies: {[key:string] Dependacy }; and here we can use:
import {
Dependency
} from '@react-native-community/cli-types';
function isValidRNDependency(
config: Dependency
) {
return (
Object.keys(config.platforms).filter(key => Boolean(config.platforms[key]))
.length !== 0 ||
(config.hooks && Object.keys(config.hooks).length !== 0) ||
(config.assets && config.assets.length !== 0) ||
(config.params && config.params.length !== 0)
);
}It would solve most of the problems breaking in this file
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.
@Esemesek is working on this? if not I would like to.
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.
@netochaves sure, please do!
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.
Should i create a new pr for this?
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.
nah, do it here
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.
Done.
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.
Nice!
10eb772 to
a19254a
Compare
thymikee
left a 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.
Thank you!
Summary:
Part of #683
Converted
commands/configto typescriptTest Plan:
Green CI