Skip to content

feat: add create plugin command #3800

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

Merged
merged 3 commits into from
Aug 22, 2018
Merged

feat: add create plugin command #3800

merged 3 commits into from
Aug 22, 2018

Conversation

lini
Copy link
Contributor

@lini lini commented Aug 7, 2018

PR Checklist

What is the new behavior?

tns plugin create command added to CLI. The command will clone the plugin seed and execute the initial seed configuration script to prepare a new project for developing a NativeScript plugin.

- `tns plugin create` command
- add documentation
- add tests
@dtopuzov
Copy link
Contributor

run ci

Copy link
Contributor

@rosen-vladimirov rosen-vladimirov left a comment

Choose a reason for hiding this comment

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

Can you please rebase your changes on the latest master, this will make your build green

const cwd = path.join(projectDir, "src");
try {
spinner.start();
await this.$childProcess.exec("npm i", { cwd: cwd });
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using childProcess.exec, you can use the $npm:
https://github.com/NativeScript/nativescript-cli/blob/master/lib/node-package-manager.ts#L18

For example:

await this.$npm.install(cwd, cwd);

}

let gitHubUsername = config.username;
if (!gitHubUsername) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can extract part of this method to a separate methods, which will make the code easier to read, for example:

const gitHubUsername = await this.getGitHubUsernam(config.username);
const pluginNameSource = await this.getPluginNameSource(config.pluginName);


const params = `gitHubUsername=${gitHubUsername} pluginName=${pluginNameSource} initGit=y`;
// run postclone script manually and kill it if it takes more than 10 sec
const outputScript = (await this.$childProcess.exec(`node scripts/postclone ${params}`, { cwd: cwd, timeout: 10000 }));
Copy link
Contributor

Choose a reason for hiding this comment

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

this could cause issues in case any of the parameters has a space in it. I recommend using childProcess.spawn:

const pathToPostCloneScript = path.join("scripts", "postclone");
const params = [ pathToPostCloneScript, `gitHubUsername=${gitHubUsername}`, `pluginName=${pluginNameSource}`, "initGit=y"];
await this.$childProcess.spawnFromEvent(process.execPath, params, "close", { cwd, timeout: 10000 });

lib/options.ts Outdated
@@ -39,7 +39,9 @@ export class Options extends commonOptionsLibPath.OptionsBase {
inspector: { type: OptionType.Boolean },
clean: { type: OptionType.Boolean },
watch: { type: OptionType.Boolean, default: true },
background: { type: OptionType.String }
background: { type: OptionType.String },
username: {type: OptionType.String},
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please format this changes (Alt + Shift +F in Visual Studio Code)

lini added 2 commits August 22, 2018 09:36
- $npm.install replaces $childProcess.exec("npm i")
- extract prompts to separate functions
- $childProcess.spawnFromEvent replaces $childProcess.exec
- formatting
- update tests
@rosen-vladimirov
Copy link
Contributor

run ci

Copy link
Contributor

@rosen-vladimirov rosen-vladimirov left a comment

Choose a reason for hiding this comment

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

Great contribution! Thank you!

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.

3 participants