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

feat/publish #1000

Merged
merged 2 commits into from
Mar 30, 2020
Merged

feat/publish #1000

merged 2 commits into from
Mar 30, 2020

Conversation

claudiahdz
Copy link
Contributor

@claudiahdz claudiahdz commented Mar 10, 2020

This PR refactors both npm publish and npm pack. Both now use libnpmpack to pack tarballs and npm publish no longer needs to write on disk to display to console the tarball contents.

@claudiahdz claudiahdz force-pushed the feat/publish branch 2 times, most recently from 0b58a78 to 3a0f260 Compare March 10, 2020 23:08
lib/utils/tar-contents.js Outdated Show resolved Hide resolved
@claudiahdz claudiahdz force-pushed the feat/publish branch 3 times, most recently from 4d8543b to 529111d Compare March 27, 2020 22:16
@claudiahdz claudiahdz marked this pull request as ready for review March 27, 2020 22:18
@claudiahdz claudiahdz requested a review from a team as a code owner March 27, 2020 22:18
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Only some pretty minor changes requested. This is looking great!

lib/pack.js Outdated Show resolved Hide resolved
lib/pack.js Outdated
}
})
.tap(() => lifecycle(pkg, 'postpack', dir))
const tarballs = await Promise.all(args.map((arg) => pack_(arg)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there's an await here, any errors will throw. Probably want to wrap this whole thing in a try { ... } catch (er) { cb(er) }

lib/publish.js Outdated Show resolved Hide resolved
lib/publish.js Outdated
defaultTag: 'latest',
json: false,
tmp: {},
...npm.flatOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

The aliasing is weird here, does libnpmpublish or libnpmpack do something with those?

Any reason you can't just pass npm.flatOptions in as the opts?

lib/publish.js Outdated Show resolved Hide resolved
lib/publish.js Show resolved Hide resolved
lib/publish.js Show resolved Hide resolved
lib/publish.js Outdated Show resolved Hide resolved
lib/utils/tar.js Outdated Show resolved Hide resolved
lib/utils/tar.js Show resolved Hide resolved
@claudiahdz claudiahdz force-pushed the feat/publish branch 3 times, most recently from b9d0625 to fbf0f01 Compare March 30, 2020 17:49
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Just need to put the usage/completion on the exported function. Otherwise looks great!

lib/pack.js Outdated Show resolved Hide resolved
lib/publish.js Outdated Show resolved Hide resolved
@claudiahdz claudiahdz merged commit bc12db1 into release/v7.0.0-beta Mar 30, 2020
@claudiahdz claudiahdz deleted the feat/publish branch March 30, 2020 21:16
@isaacs
Copy link
Contributor

isaacs commented Mar 31, 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