feat: pnpm store add command by etamponi · Pull Request #1268 · pnpm/pnpm

etamponi · 2018-07-09T17:14:53Z

No description provided.

zkochan · 2018-07-09T19:04:29Z

+
+  const pkgIds = []
+  for (const dep of deps) {
+    const ret = await opts.storeController.requestPackage(dep, {


I think you can run them all simultaneously because that is how it works during regular installation (there is a network concurrency limit inside package-requester).

const pkgIds = await Promise.all(deps.map(...

zkochan · 2018-07-09T19:10:05Z

+  const deps = await parseWantedDependencies(fuzzyDeps, {
+    allowNew: true,
+    currentPrefs: {},
+    defaultTag: '',


you could pass in the tag config from pnpm. It is a config that is "latest" by default.

zkochan · 2018-07-09T19:14:34Z

+    pkgIds.push(ret.body.id)
+  }
+
+  await opts.storeController.updateConnections(opts.prefix, {


As far as I remember, this is not the desired behavior because you don't want this prefix to be added to store.json

I think what we can do is, make package requester add the new fields to storeIndex right after downloading the packages to the store.

Then calling updateConnections won't be needed

As you can see, storeIndex is passed in to package-requester
https://github.com/pnpm/package-store/blob/master/src/storeController/index.ts#L55

so you can just add the field in package-requester like it is done in package-store currently (but just add an empty array)

https://github.com/pnpm/package-store/blob/master/src/storeController/index.ts#L91

I am not sure what you mean with this

with the implementation you did, an entry like this well be added to store.json:

"registry.npmjs.com/foo/1.0.0": ["/home/usr"],

but what you need is:

"registry.npmjs.com/foo/1.0.0": [],

zkochan · 2018-07-09T19:17:07Z

+  await storeAdd(['express@4.16.3'], opts as any) // tslint:disable-line
+
+  await project.storeHas('express', '4.16.3')
+})


might be good to also test the content of store.json

storeHas already checks the content of store.json

etamponi · 2018-07-10T07:27:32Z

That's the part I understood. The part I didn't understand is if I have to add an entry everytime I call requestPackage, or only in this specific case, that is, when used inside the store add command. In both cases, I am not entirely sure this makes sense. I would rather add a way for updateConnections to add an empty array. For example if prefix is null or empty, it adds an empty array. Il mar 10 lug 2018, 09:22 Zoltan Kochan <notifications@github.com> ha scritto:

…

***@***.**** commented on this pull request. ------------------------------ In packages/supi/src/api/storeAdd.ts <#1268 (comment)>: > + const pkgIds = [] + for (const dep of deps) { + const ret = await opts.storeController.requestPackage(dep, { + downloadPriority: 1, + loggedPkg: { + rawSpec: dep.raw, + }, + preferredVersions: {}, + prefix: opts.prefix, + registry: normalizeRegistryUrl(opts.registry || 'https://registry.npmjs.org/'), + verifyStoreIntegrity: opts.verifyStoreIntegrity, + }) + pkgIds.push(ret.body.id) + } + + await opts.storeController.updateConnections(opts.prefix, { with the implementation you did, an entry like this well be added to store.json: "registry.npmjs.com/foo/1.0.0": ["/home/usr"], but what you need is: "registry.npmjs.com/foo/1.0.0": [], — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1268 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjUNHax4tLIvnLpVp09Rr1fWkc8ahjcks5uFFZNgaJpZM4VIFKI> .

zkochan · 2018-07-10T07:34:05Z

I meant that requestPackage would add the empty array in all cases. I think it would make sense because the package is in the store but it does not have any connections yet

etamponi · 2018-07-10T07:35:46Z

Oh, ok Il mar 10 lug 2018, 09:34 Zoltan Kochan <notifications@github.com> ha scritto:

…

I meant that requestPackage would add the empty array in all cases. I think it would make sense because the package is in the store but it does not have any connections yet — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1268 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjUNHvvov-lhfTgP2rLK3b2x1um4t4Nks5uFFjzgaJpZM4VIFKI> .

etamponi · 2018-07-10T08:15:07Z

I'll add the test that checks store.json after you release the new version of package-requester

PR #26 ref pnpm/pnpm#1268

zkochan · 2018-07-10T10:39:17Z

+  const deps = await parseWantedDependencies(fuzzyDeps, {
+    allowNew: true,
+    currentPrefs: {},
+    defaultTag: opts.tag || 'default',


the default should be 'latest'

zkochan · 2018-07-10T10:51:39Z

+      registry: normalizeRegistryUrl(opts.registry || 'https://registry.npmjs.org/'),
+      verifyStoreIntegrity: opts.verifyStoreIntegrity || true,
+    })
+    await pkgResponse['fetchingFiles'].catch((err: Error) => {}) // tslint:disable-line


I think there should be an info message printed about the added tarballs.
When you run pnpm store add typescript@1 it would be useful to see which version of typescript was added. So maybe just print the pkgId?

$ pnpm store add typescript@1 + registry.npmjs.org/typescript/1.2.1

pnpm store prune provides a similar output at the moment.

Also, I think it is important to print failures. So don't just ignore if fetchingFiles fails. Maybe the process should exit with a non-zero exit code. Fetching the package is the intention of the operation after all, so if it fails, the operation was not successful.

zkochan · 2018-07-10T10:52:47Z

+  await project.storeHas('express', '4.16.3')
+
+  const storeIndex = await loadJsonFile(path.join(opts.store, 'store.json'))
+  t.deepEqual(storeIndex['localhost+4873/express/4.16.3'], [])


You can make a deepEqual of the whole file because it should be empty

t.deepEqual(storeIndex, { 'localhost+4873/express/4.16.3': [], })

it also contains another package, I needed to install it to initialize the store in the test

oh, I see. But that's an issue, isn't it? I think pnpm store add should be able to create a new store

It only happens in this test, if I run it from the command line it creates the store correctly

are you sure? When you run it manually, it probably uses your global store that already exists at ~/.pnpm-store but tests use a new store in every test case. So to test it manually, you'd need something like rm -rf store && pnpm store add typescript@1 --store store

That's exactly how I tested it

zkochan · 2018-07-10T10:53:43Z

        Returns a 0 exit code if packages in the store are not modified, i.e. the content of the package is the
        same as it was at the time of unpacking.

+        pnpm store add


pnpm store add [pkg...] would be more straightforward

zkochan · 2018-07-10T11:22:22Z

+  }
+
+  if (hasFailures) {
+    throw new Error('Some packages have not been added correctly')


please add an additional test for checking this behavior. You can just add a non-existent package to the store pnpm store add @pnpm/this-does-not-exist

zkochan · 2018-07-10T11:57:08Z

+          rawSpec: dep.raw,
+        },
+        preferredVersions: {},
+        prefix: '',


should it work with local tarballs? If you pass in the prefix, it will work with pnpm store add ../some.tgz

I have no idea what the prefix is

it should be passed in by pnpm. prefix is basically the current working directory in most cases

zkochan · 2018-07-11T20:54:36Z

🚢 2.12.0-0

feat: pnpm store add command

33baf19

etamponi requested a review from zkochan as a code owner July 9, 2018 17:14

Emanuele Tamponi added 2 commits July 9, 2018 19:26

test: add tests, lint

c06aa48

test: fix storeAdd test

7381c89

zkochan reviewed Jul 9, 2018

View reviewed changes

fix: various fixes following review

5441226

zkochan pushed a commit to pnpm/package-requester that referenced this pull request Jul 10, 2018

fix: add package to store index after fetching it

9b38a95

PR #26 ref pnpm/pnpm#1268

fix: bump up dependencies, test for store.json content

e853cdc

zkochan reviewed Jul 10, 2018

View reviewed changes

Emanuele Tamponi added 2 commits July 10, 2018 13:02

fix: additional changes from review

31618a1

fix: linting errors

fd688d8

zkochan reviewed Jul 10, 2018

View reviewed changes

test: add test for storeAdd failure condition

7c844a5

zkochan reviewed Jul 10, 2018

View reviewed changes

Emanuele Tamponi and others added 3 commits July 10, 2018 15:12

fix: pass prefix

6113039

test: remove dummy install before testing storeAdd()

a7fb3da

test: pnpm store add

bf8db84

zkochan approved these changes Jul 10, 2018

View reviewed changes

zkochan merged commit 7bebfb8 into master Jul 10, 2018

zkochan deleted the store-add-cmd branch July 10, 2018 21:10

Uh oh!

Conversation

etamponi commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etamponi commented Jul 10, 2018 via email

Uh oh!

zkochan commented Jul 10, 2018

Uh oh!

etamponi commented Jul 10, 2018 via email

Uh oh!

etamponi commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etamponi Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkochan commented Jul 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

etamponi Jul 10, 2018 •

edited

Loading