fix: overrides in release option #345
Conversation
|
i would prefer not to run additional pnpm calls, esp. pnpm remove. is there an equivalent of |
no we do not have that. that's why we should install first! |
|
what would it take to add that? tbh the changes needed to accommodate pr.pkg.new start to get me to believe that just always building vite is the way to go, or investigate a separate persisted hosted registry where ecosystem-ci can cache its own builds and query dependencies as needed (vue-ecosystem-ci does that i believe) |
I'm trying to find a way to skip the current |
utils.ts
Outdated
| @@ -274,6 +274,19 @@ export async function runInRepo(options: RunOptions & RepoOptions) { | |||
| } else { | |||
| overrides.vite = options.release | |||
| } | |||
| const viteManifest = JSON.parse( | |||
| await $`pnpm dlx pacote manifest ${options.release} --json`, | |||
There was a problem hiding this comment.
@dominikg pacote is what npm info uses, so i added a call for it here to get the manifest of the release.
There was a problem hiding this comment.
why dlx instead of adding it to the repo as dev dependency and calling manifest directly? But i thought you said that pkg.pr.new didn't support the info service?
There was a problem hiding this comment.
why dlx instead of adding it to the repo as dev dependency and calling manifest directly?
I can do it if you want! let me send a commit.
But i thought you said that pkg.pr.new didn't support the info service?
pacote does not rely on any service, it supports standalone tarballs and also packages from the registry.
There was a problem hiding this comment.
this would allow adding the override without running install first at the expense of downloading the full tarball just to inspect its package.json 🙈 . I was under the impression that npm info had a more elegant way to get to that information.
There was a problem hiding this comment.
and yes, please don't use dlx, we want control over the version of pacote that is used and its more efficient to take it from local.
There was a problem hiding this comment.
I was under the impression that npm info had a more elegant way to get to that information.
Yea, that'd be costly for servers if they want to consume the tarball themselves and parse the json! good for small packages, but for heavy packages, that's too much to unpack a tarball in the memory. Specially if that's cloudflare workers, which is what pkg.pr.new uses :)
we want control over the version of pacote that is used and its more efficient to take it from local.
Noted, in the last commit.
There was a problem hiding this comment.
but for heavy packages, that's too much to unpack a tarball in the memory.
I think you can process it in stream and only extract the package.json so that you won't need to keep the whole unpacked tarball. It's a bit hard though.
utils.ts
Outdated
| @@ -274,6 +275,17 @@ export async function runInRepo(options: RunOptions & RepoOptions) { | |||
| } else { | |||
| overrides.vite = options.release | |||
| } | |||
| const viteManifest = await pacote.manifest(options.release) | |||
There was a problem hiding this comment.
this should only be called if we need the manifest to determine the value for an override.
There was a problem hiding this comment.
ok pushed a new change, we'd have to call pacote in each if statement, but i think that's ok since it caches the results!
utils.ts
Outdated
| @@ -274,6 +274,19 @@ export async function runInRepo(options: RunOptions & RepoOptions) { | |||
| } else { | |||
| overrides.vite = options.release | |||
| } | |||
| const viteManifest = JSON.parse( | |||
| await $`pnpm dlx pacote manifest ${options.release} --json`, | |||
There was a problem hiding this comment.
but for heavy packages, that's too much to unpack a tarball in the memory.
I think you can process it in stream and only extract the package.json so that you won't need to keep the whole unpacked tarball. It's a bit hard though.
Co-authored-by: 翠 / green <[email protected]>
that's an interesting idea! I think that'd be doable, but it takes time as you say. maybe we can have a different service that extracts specific files from a tarball on the fly :) |
|
do we really want to treat rollup and esbuild override flags differently (one must be explicit false the other explicit true?) from a usage perspective it would be nicer if they behaved the same. for pacote even if it caches internally (how, where?) i'd prefer making that call only once, so checking both conditions first, if either is met then run pacote, then act on the output. |
My rationale here is to minimize the difference from the actual users. Since we only override esbuild for marko, I think it's better not to override esbuild for others. BTW I wonder if we can remove the need of overriding rollup in most cases by calling |
With #313, few tests like the marko tests were failing to invalid overrides! those overrides were relying on the vite workspace, even though with pkg.pr.new, there's no vite cloning, so that was wrong.
In this pr, we install vite from pkg.pr.new and we use rollup/esbuild versions from there as overrides for tests like marko!