feat: publish netlify package
#7293
Conversation
📊 Benchmark resultsComparing with 298a379
|
scripts/netlifyPackage.js
Outdated
| const newPackageJSON = { | ||
| ...packageJSON.contents, | ||
| main: './dist/index.js', | ||
| name: 'netlify', |
There was a problem hiding this comment.
Now I'm wondering if we should also change the name of the binary so that npx netlify and any globally-installed netlify-cli do not collide?
There was a problem hiding this comment.
As far as I understand the wonky npm heuristics here:
- if a user has both
netlifyandnetlify-cliinstalled (both locally or both globally),npx netlifywill runnetlify - if a user has
netlify-cliand does not havenetlify,npx netlifywill always runnetlify-clirather than try to installnetlify - if a user has neither installed,
npx netlifywill install and runnetlify
(It's likely worth verifying these)
Does this seem ok or are any of these scenarios problematic?
There was a problem hiding this comment.
My question is whether npx netlify will also install the binary into the PATH, in the same way that globally-installing a package does. If that's the case, then the netlify binary will overwrite the binary with the same name that had been injected by a globally-installed netlify-cli, which is not intended.
But if npx netlify simply runs the binary without adding it to the PATH (which I feel is the expected behaviour), this should be fine.
There was a problem hiding this comment.
nit: I was going to comment that this codebase does not follow this naming convention, but I see that scripts/prepublishOnly is the one exception, which you probably saw right next to this one!
🤷🏼
There was a problem hiding this comment.
I did name scripts/prepublishOnly as it is (despite not matching other file naming conventions) specifically so it would match up exactly with the npm script name.
|
|
||
| async function getPackageJSON() { | ||
| const packageJSONPath = resolve(fileURLToPath(import.meta.url), '../../package.json') | ||
| const contents = JSON.parse(await readFile(packageJSONPath, 'utf8')) |
There was a problem hiding this comment.
nit: We already have a dep on normalize-package-data you can use here to avoid contents being any
There's even a util src/utils/get-cli-package-json.ts you should be able to use
There was a problem hiding this comment.
This use case is a bit different. We're reading package.json and then writing it back, so I don't think we should be normalising it.
I have used the Package type now.
| @@ -0,0 +1,4 @@ | |||
| // This is an entrypoint that mirrors the interface that the `netlify` package | |||
There was a problem hiding this comment.
thought: Should we log a warning here that the package has been renamed?
There was a problem hiding this comment.
That feels a bit extreme? We might be logging something that is not actionable to the user, since this might be a transitive dependency.
scripts/netlifyPackage.js
Outdated
| console.log('Re-installing dependencies to update lockfile...') | ||
| await execa('npm', ['install'], { | ||
| cwd: dirname(packageJSON.path), | ||
| }) |
There was a problem hiding this comment.
Hrm, his is getting difficult to reason about... So, before this script runs in CI, we'll have run this as part of the netlify-ci publish step: https://github.com/netlify/cli/blob/47242ce76c7aa57978e14e9e075ca6f0f76af348/scripts/prepublishOnly.js.
This means we'll have mutated package.json to prune dev deps, converted the package-lock.json to an npm-shrinkwrap.json, then restored the original package.json... now here we'll mutate package.json again to update name and add main, then run npm i again, which will restore the dev deps into the npm-shrinkwrap.json, making netlify twice as big to install as netlify-cli. 😰 Did I get all that right?
@ndhoule and I just recently decomplicated all of this (with many false starts due to how tangled and brittle it was), so I think it would be worth avoiding getting back into this state if we can help it. I don't have concrete suggestions that fit in a comment box, but I'm happy to pair on it if you'd like.
There was a problem hiding this comment.
Okay, I hadn't realised that this flow had changed. My bad.
In 70dacdb I have made the following changes:
- Changed
prepublishOnly.jsto stop trying to restore the original state ofpackage.json. This flow is meant to run in the CI only, where trying to restore things has no benefit and will only slow things down. - Changed
netlifyPackage.jsto runnpm shrinkwrap(instead ofnpm install) so that the shrinkwrap file is updated with the right package name. It will have the same dependencies, as we've preserved thepackage.jsonfile modified in step 1.
Let me know if that makes sense.
scripts/netlifyPackage.js
Outdated
| const newPackageJSON = { | ||
| ...packageJSON.contents, | ||
| main: './dist/index.js', | ||
| name: 'netlify', |
There was a problem hiding this comment.
As far as I understand the wonky npm heuristics here:
- if a user has both
netlifyandnetlify-cliinstalled (both locally or both globally),npx netlifywill runnetlify - if a user has
netlify-cliand does not havenetlify,npx netlifywill always runnetlify-clirather than try to installnetlify - if a user has neither installed,
npx netlifywill install and runnetlify
(It's likely worth verifying these)
Does this seem ok or are any of these scenarios problematic?
Summary
Adds a script that publishes the same code as a second package (
netlify). We have different options to make this happen, like makingnetlify-clia dependency ofnetlify. But to minimise the changes tonetlify-cli(which doesn't even have amainfield at the moment), I went for a script that is meant to be run in the CI and patchespackage.jsonwith the new name andmainbefore publishing it.