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

Add support for npm publish --dry-run #8

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

Jskobos
Copy link
Contributor

@Jskobos Jskobos commented Jul 21, 2020

From #6: Add an extra Option/command line argument to optionally run npm publish with the --dry-run flag enabled to prevent
actual publishing. Intended for use in testing workflows. This option/argument defaults to false.

Wasn't sure if build files should be committed, so I left them out for now.

Add an extra Option/command line argument to optionally
run npm publish with the --dry-run flag enabled to prevent
actual publishing. Intended for use in testing workflows.
This option/argument defaults to false.
Copy link
Member

@JamesMessinger JamesMessinger left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and especially for writing so many great tests! I really appreciate that.

Everything looks good. I requested one change, and then I look forward to merging the PR. Great job!

@@ -25,7 +25,7 @@ export async function npmPublish(opts: Options = {}): Promise<Results> {

let results: Results = {
package: manifest.name,
type: diff || "none",
type: options.dryRun ? "dry-run" : (diff || "none"),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of overloading the type property, let's add a new property to the Results object (i.e. dryRun: boolean). That way, scripts can still check the type property like normal, regardless of whether it was a dry run or not.

debug: debugHandler,
};

// Puglish to NPM
// Publish to NPM
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for fixing my typos. 🤦‍♂️

@@ -25,7 +25,7 @@ export const npm = {

options.debug(`Running command: npm view ${name} version`);

// Run NPM to get the latest published versiono of the package
// Run NPM to get the latest published version of the package
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for fixing my typos. 🤦‍♂️

@Jskobos
Copy link
Contributor Author

Jskobos commented Jul 23, 2020

@JamesMessinger thanks for the feedback.

Copy link
Member

@JamesMessinger JamesMessinger left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. Looks great!

@JamesMessinger JamesMessinger merged commit e1e417b into JS-DevTools:master Jul 23, 2020
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.

2 participants