-
Notifications
You must be signed in to change notification settings - Fork 69
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: add custom wrapper for publishing #727
Conversation
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
scripts/publish.js
Outdated
const fs = require('node:fs/promises') | ||
const util = require('node:util') | ||
const exec = util.promisify(require('node:child_process').exec) | ||
const { spawn } = require('node:child_process') |
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.
exec, because it's easy to promisify and we read the output as json
spawn, because we want to print the npm output to console (shows what was packed into the tarball)
scripts/publish.js
Outdated
spawn('npm', args, { | ||
cwd: ROOT, | ||
stdio: 'inherit', | ||
shell: true, |
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.
don't love this, but we need it for nvm. otherwise we will need to use something like which
to find it
scripts/publish.js
Outdated
/** | ||
* A JSON.parse that doesn't mind a rejected promise | ||
* @param {Promise<string>} promise | ||
* @returns {Promise<any>} | ||
*/ | ||
const chillJSONParse = async (promise) => | ||
promise | ||
.catch((err) => { | ||
console.error(err.message) | ||
return '{}' | ||
}) | ||
.then(JSON.parse) | ||
|
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.
@naugtur So if this parsing fails, either a) package.json
is invalid, or b) npm show --json
is invalid, and we should continue but with a message to the user that they may need to publish this package manually.
I don't know why we'd want to continue normally if a package.json
is invalid though.
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.
If package.json is not found or invalid, the result of parsing is an empty object, the error was printed and the package was skipped.
I introduced it meaning to complete publishing what makes sense and let you fix and rerun to publish the thing that was broken before.
But I think I was making a wrong assumption - if we throw an error from parsing package.json - the whole publish is not happening and the release can be fixed before another attempt.
I've lost confidence in my alleged improvement 😅
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.
Let's drop the chillJSONParse function and use regular JSON.parse in a big try-catch in the map callback to add a message explaining the package from the folder was not fit to publish and then adding the rest of the error for debugging.
scripts/publish.js
Outdated
) | ||
if (private) { | ||
if (private || !name) { |
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.
if name
is undefined, that's an invalid package.json
file and we should probably say something
scripts/publish.js
Outdated
}) | ||
|
||
const versions = JSON.parse(stdout) | ||
const versions = await chillJSONParse( |
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.
why mix await and then/catch? these are all wrapped in a Promise.all()
, so async operations should not happen in serial anyhow
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.
it was for readability. the function passed to map is async, so this will not turn anything into a sequence.
443098d
to
dafab24
Compare
* @property {boolean} [private] | ||
* @property {string} name | ||
* @property {string} version | ||
*/ |
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.
(could) This might work better if put in a d.ts 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.
will look at that if/when we end up transitioning this to a package.
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.
This is solid. We're gonna have to publish it to the ecosystem after we use it for a little while.
dafab24
to
b14a863
Compare
Added some checks around the |
b14a863
to
6d6de08
Compare
Closes #704 |
The aim here is to wholly replace Lerna, since we're no longer using it for much.
Unfortunately,
npm publish
doesn't have any sort of flag to ignore already-published packages, so here we are.The script:
package.json
package.json
has already been published to the npm registry--newPkg <pkg-name>
option was provided to the scriptnpm publish
once with arg--workspace <pkg-name>
for every package that needs publishing. Output is piped thru, so you can examine the results ofnpm pack
before they go outThere's a decent test suite here, too.
The script has some basic
--help
output, and supports the following options:--root <path>
- Specify the workspace root directory. Default is just the parent dir of the script's dir--dryRun
- Runnpm publish
in "dry run" mode--newPkg <name>
- Workspace package with namename
has not yet been published to the registry