Closed etamponi closed 6 years ago
Then how do I trigger a full rebuild?
Il 28 dic 2017 15:34, "Zoltan Kochan" notifications@github.com ha scritto:
@zkochan commented on this pull request.
I don't think I have objections, so you can work on the tests. I will review again, more thoroughly tonight.
In src/api/install.ts https://github.com/pnpm/supi/pull/30#discussion_r158950614:
@@ -521,6 +521,23 @@ async function installInContext ( outdatedPkgs: installCtx.outdatedPkgs, })
- let packagesToBuild: object[] = []
- let hasPackagesToBuild = false
- if (opts.ignoreScripts) {
- hasPackagesToBuild = true
- packagesToBuild = R.props<string, DependencyTreeNode>(result.newPkgResolvedIds, result.linkedPkgsMap)
isn't it enough just to save an array of package IDs?
In src/api/install.ts https://github.com/pnpm/supi/pull/30#discussion_r158950892:
@@ -531,13 +548,14 @@ async function installInContext ( skipped: Array.from(installCtx.skipped), layoutVersion: LAYOUT_VERSION, independentLeaves: opts.independentLeaves,
- hasPackagesToBuild: hasPackagesToBuild,
why is this needed? if the array value has any elements, it means there are packages to build
In src/api/install.ts https://github.com/pnpm/supi/pull/30#discussion_r158951085:
@@ -531,13 +548,14 @@ async function installInContext ( skipped: Array.from(installCtx.skipped), layoutVersion: LAYOUT_VERSION, independentLeaves: opts.independentLeaves,
- hasPackagesToBuild: hasPackagesToBuild,
- packagesToBuild: packagesToBuild,
maybe a better name would be pendingBuilds.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/pull/30#pullrequestreview-85828132, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNB-zdxwlS_7pqlXvUESrvPQs0c-iks5tE6b0gaJpZM4ROXOT .
Maybe full rebuild should stay the default behavior and a new command should be added. Something like pnpm rebuild --pending
Alright :+1: I'll do it
I am not using package ids so that I can store the package list in the same format used by the rebuild method, so it matches the type PackageToRebuild. Perhaps I can use the getPackagesInfo method on package ids instead?
Il 28 dic 2017 15:40, "Zoltan Kochan" notifications@github.com ha scritto:
Maybe full rebuild should stay the default behavior and a new command should be added. Something like pnpm rebuild --pending
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/pull/30#issuecomment-354298510, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNJYiKbVNx1K8ie0HK_7_tOhI6PW9ks5tE6hUgaJpZM4ROXOT .
I can't look into it now but there is a "resolved package ID" or dependency path, the thing that is the key in shrinkwrap.packages[]
. You should save that in the array. The difference between package ID and this other thing is that it can have the same package@version
several times if the package has peer dependencies.
I think I have it well documented here:
https://github.com/pnpm/spec/blob/master/package-id.md https://github.com/pnpm/spec/blob/master/dependency-path.md
Alright then I'll try to use that second ID instead of the one I am using right now
Il 28 dic 2017 15:52, "Zoltan Kochan" notifications@github.com ha scritto:
I can't look into it now but there is a "resolved package ID" or dependency path, the thing that is the key in shrinkwrap.packages[]. You should save that in the array. The difference between package ID and this other thing is that it can have the same package@version several times if the package has peer dependencies.
I think I have it well documented here:
https://github.com/pnpm/spec/blob/master/package-id.md https://github.com/pnpm/spec/blob/master/dependency-path.md
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/pull/30#issuecomment-354300176, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNOpymoMYeNzYi-fWrPFg_Dox5gNnks5tE6slgaJpZM4ROXOT .
Another issue: pnpm rebuild
does not run the install hooks of the root. Can I change this?
If that is how npm rebuild
works, then please change it.
Alright.
I have no idea how to get the keys used in shrinkwrap.packages
in the install step... Ideas welcome
Doesn't this have the needed ids? result.newPkgResolvedIds
Already tried... Nope, it contains the absolute ids, which also contain the registry, so they can't be used as keys of shrinkwrap.packages
.
Example: registry.npmjs.org/to-array/0.1.4
, but in shrinkwrap.packages
it's /to-array/0.1.4
.
Perhaps there is a resolution method to convert between the two?
oh, yes. There is. The dependencyPath.relative(registry, 'registry.npmjs.org/foo/1.0.0')
from https://github.com/pnpm/dependency-path
The default registry is in shrinkwrap.registry
Yup, just found :) Thanks!
On uninstall (/api/uninstall.ts
), the removed packages should be removed from node_modules/.modules.yaml
. Same way the skipped
is cleaned up.
Code updated to make sure the pendingBuilds
are up to date both when installing and when uninstalling (and removing orphans).
I'll add tests tomorrow
Running the whole test suite every time is really time consuming, doesn't allow me to write the tests really quickly... is there a way to only run specific test cases, or at least a single test file?
EDIT: ok, I figured this one out too :P
I've added a test for the --pending
option. I am not 100% sure how to test that it's actually doing stuff incrementally, suggestions welcome!
Let me know if you'd like to have more tests, and which ones :)
Also, to one of the existing tests, add a check that the pendingBuilds
is not populated when running install w/o the --ignore-scripts opiont
I think I am done
Can it? I thought the list would be free of duplicates because of how it is constructed: we only concat new packages, so duplicates should never be there. Anyway adding an additional call to ensure this might be wise.
Il 29 dic 2017 21:01, "Zoltan Kochan" notifications@github.com ha scritto:
In src/api/install.ts https://github.com/pnpm/supi/pull/30#discussion_r159098931:
@@ -521,23 +522,31 @@ async function installInContext ( outdatedPkgs: installCtx.outdatedPkgs, })
In this case duplication might appear pendingBuilds. R.union() can concat them removing duplicates.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/pull/30#pullrequestreview-86004470, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNOr13Qf1io5eaoSvBUsWAw_jSacDks5tFUUYgaJpZM4ROXOT .
oh, right. I guess you are right. There won't be duplicates. Well, in that case just add a comment, that there will never be duplicates, so concat is fine
Here you go.
If you like it, let me know if I have to add some tests :)