side effects cache #38

etamponi · 2018-01-16T16:12:50Z

No description provided.

zkochan

Should be covered with tests

zkochan · 2018-01-16T18:57:06Z

src/api/extendInstallOptions.ts

@@ -124,6 +128,8 @@ const defaults = async (opts: InstallOptions) => {
    verifyStoreIntegrity: true,
    hooks: {},
    savePrefix: '^',
+    sideEffectsCache: false,


We'll probably make it true by default in the future

zkochan · 2018-01-16T18:58:33Z

src/api/install.ts

@@ -573,13 +575,19 @@ async function installInContext (
        R.props<string, DependencyTreeNode>(result.newPkgResolvedIds, result.linkedPkgsMap)
          .map(pkg => limitChild(async () => {
            try {
-              await postInstall(pkg.peripheralLocation, {
+              if (pkg.importedFromCache) {


move this check to a filter function before .map()

zkochan · 2018-01-16T19:04:03Z

src/link/index.ts

  }
 ) {
  return Promise.all(
    alldeps.map(async pkg => {
      const filesResponse = await pkg.fetchingFiles

      if (pkg.independent) return
-      return storeController.importPackage(pkg.centralLocation, pkg.peripheralLocation, {
+
+      let importFrom = pkg.centralLocation


just change the value of centralLocation to the path that will be used here, so move this logic with sideEffectsCache to resolvePeers and assign centralLocation the correct value

zkochan · 2018-01-16T19:06:21Z

src/link/resolvePeers.ts

@@ -53,6 +53,8 @@ export type DependencyTreeNode = {
    cpu?: string[],
    os?: string[],
  },
+  sideEffectsCache: Map<string, string>,
+  importedFromCache?: boolean,


Might be better to be more specific here, so importedFromSideEffectsCache or isBuilt or skipScripts

zkochan · 2018-01-18T19:46:24Z

src/install/installMultiple.ts

@@ -403,7 +408,8 @@ async function install (
        engines: pkg.engines,
        cpu: pkg.cpu,
        os: pkg.os,
-      }
+      },
+      sideEffectsCache: pkgResponse.body.sideEffectsCache,


Just assign one value to it

sideEffectsCacheLocation: pkgResponse.body.sideEffectsCache[nodeMajor],

also, I'd suggest renaming the map to pkgResponse.body.sideEffectsCacheByEngine or pkgResponse.body.sideEffectsCacheByNodeVersion

I think it would be even fine to make it shorter pkgResponse.body.cacheByNodeVersion, as this is the only type of cache that depends on Node/engine

also, when options.force is true, then always assign null to the sifeEffectsCacheLocation. In that case, cache will be rewritten

zkochan · 2018-01-18T19:53:34Z

src/link/resolvePeers.ts

+    const nodeMajor = process.version.substring(0, process.version.indexOf('.'))
+    if (node.sideEffectsCache && node.sideEffectsCache[nodeMajor]) {
+      node.isBuilt = true
+      node.centralLocation = node.sideEffectsCache[nodeMajor]


assign these two values right where the node is created:

supi/src/link/resolvePeers.ts

Line 182 in b7dae8b

ctx.resolvedTree[absolutePath] = {

it is much easier to read the code when all properties of the object are set in one place and stay immutable

I couldn't find where the node was created, that's why :) Ok 👍

zkochan · 2018-01-19T17:40:10Z

src/install/installMultiple.ts

@@ -409,7 +410,7 @@ async function install (
        cpu: pkg.cpu,
        os: pkg.os,
      },
-      sideEffectsCache: pkgResponse.body.sideEffectsCache,
+      cacheByNodeVersion: ctx.force ? '' : pkgResponse.body.sideEffectsCache[nodeMajor],


you can make this property nullable and assign it null or undefined. The empty string doesn't make sense

zkochan · 2018-01-19T17:44:51Z

src/link/resolvePeers.ts

@@ -185,7 +179,8 @@ function resolvePeersOfNode (
      hasBundledDependencies: node.pkg.hasBundledDependencies,
      fetchingFiles: node.pkg.fetchingFiles,
      resolution: node.pkg.resolution,
-      centralLocation,
+      centralLocation: node.pkg.cacheByNodeVersion || centralLocation,


You'll have to change it here:

supi/src/link/resolvePeers.ts

Line 172 in b8bc8cf

const centralLocation = path.join(node.pkg.path, 'node_modules', node.pkg.name)

centralLocation is used for peripheralLocation sometimes, so it should have the correct value

zkochan · 2018-01-19T17:45:57Z

src/install/installMultiple.ts

@@ -385,6 +385,7 @@ async function install (

    const peerDependencies = peerDependenciesWithoutOwn(pkg)

+    const nodeMajor = process.version.substring(0, process.version.indexOf('.'))


move this line to the beginning of the file. It is enough to calculate nodeMajor once. There is no need to recalculate it on each function call

…Major to the top

zkochan · 2018-01-19T19:26:17Z

src/install/installMultiple.ts

@@ -403,7 +410,8 @@ async function install (
        engines: pkg.engines,
        cpu: pkg.cpu,
        os: pkg.os,
-      }
+      },
+      cacheByNodeVersion: ctx.force || pkgResponse.body.sideEffectsCache[NODE_MAJOR],


actually, it can be simplified even more.

This object has a property called path. path is used only once, when centralLocation is assigned. So central location can be calculated here, instead of in resolvePeers.ts. In that case this object will need to contain one property instead of two. So only centralLocation instead of path and cacheByNodeVersion

And how do we mark isBuilt? Do we really need to do this? It seems more complicated to me.

isBuilt can be added to this object as well

but maybe lets wait with this. I am still thinking about our discussion in package-requester. If the cache can depend on dependency set, this code will have to be changed more and in a different place

So we are replacing one field for another? I started doing what you asked for and I had to add way more lines of code than those I removed... Should I go ahead? Really?

…ache

zkochan · 2018-01-20T16:29:26Z

src/install/postInstall.ts

@@ -36,7 +36,8 @@ export default async function postInstall (
  await npmRunScript('preinstall', pkg, scriptsOpts)
  await npmRunScript('install', pkg, scriptsOpts)
  await npmRunScript('postinstall', pkg, scriptsOpts)
-  return
+
+  return !!scripts['preinstall'] || !!scripts['install'] || !!scripts['postinstall']


this is too much I think. We only want to cache compilation of native libs. Some lifecycle script might do necessary things. Like husky configures git hooks

Well, if there were an easy way to only cache that, it would be great. Is there? Perhaps we could only cache preinstall and install, and always run postinstall.

Again, this is what zeit claims to do, I am sticking with that.

EDIT: additionally, I would like to cache everything, not only compilation of native libs... my goal is to make installation very very fast on Glitch projects, so I'd like to cache as much as possible.

etamponi · 2018-01-21T17:25:54Z

Question: --side-effects-cache-readonly should automatically turn --side-effects-cache to true, right? Like, I shouldn't need to do pnpm install --side-effects-cache --side-effects-cache-readonly, but just pnpm install --side-effects-cache-readonly, right?

zkochan · 2018-01-21T17:28:20Z

Vice versa, --side-effects-cache should turn --side-effects-cache-readonly to true

etamponi · 2018-01-21T17:28:54Z

What?

etamponi · 2018-01-21T17:30:24Z

Let me clarify:

pnpm install --side-effects-cache: use & create side effects cache
pnpm install --side-effects-cache-readonly: use & NOT create side effects cache
pnpm install --side-effects-cache --side-effects-cache-readonly: same as previous, but redundant.

That's what I would expect the behaviour to be.

zkochan · 2018-01-21T17:31:57Z

yes, I agree. The 3rd one could be even an error or a warning

zkochan

I'd suggest one more test that would verify that the side effects cache is overwritten when installing with force: true.

It is possible to test it by comparing one of the files' ino number before/after the second install

zkochan · 2018-01-21T18:16:45Z

test/install/sideEffects.ts

+  await installPkgs(['runas@3.1.1'], opts)
+
+  // Modify the side effects cache to make sure we are using it
+  // TODO this test won't work anymore when we introduce integrity.json


it will be possible to make it work by specifying verifyStoreIntegrity: false

zkochan · 2018-01-21T18:21:28Z

We should probably add "node" to the side effects cache dirname because it's currently unclear what the v4 at the end means. So something lik linux-x64-node-v4

zkochan · 2018-01-22T00:12:16Z

🚢 0.12.0

Emanuele Tamponi added 2 commits January 16, 2018 11:37

feat: upload side effects cache when needed

62c0165

feat: add side-effects-cache support

f513ee0

zkochan suggested changes Jan 16, 2018

View reviewed changes

fix: review suggestions

b7dae8b

zkochan reviewed Jan 18, 2018

View reviewed changes

fix: rename to cacheByNodeVersion, compute immediately centralLocation

b8bc8cf

zkochan reviewed Jan 19, 2018

View reviewed changes

fix: make cacheByNodeVersion nullable, fix centralLocation, move node…

8d2bcab

…Major to the top

zkochan reviewed Jan 19, 2018

View reviewed changes

Emanuele Tamponi and others added 4 commits January 19, 2018 21:20

fix: s/nodeVersion/engineName/

7987919

fix: some renames, add verifyStoreIntegrity option

b497922

fix(deps): update

f3fe4d8

Merge remote-tracking branch 'origin/master' into feat/side-effects-c…

b5ba87f

…ache

zkochan reviewed Jan 20, 2018

View reviewed changes

zkochan added 3 commits January 20, 2018 18:57

fix: fix failing tests

a0a1a03

fix: update package-store to v0.15.1

7fbe307

test: creating side effects cache

4e997c3

test: using the side effects cache, in readonly mode too

4ba18d2

zkochan reviewed Jan 21, 2018

View reviewed changes

fix: rename engine to include 'node', test for opts.force

b55934c

zkochan merged commit 0985078 into master Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

side effects cache #38

side effects cache #38

etamponi commented Jan 16, 2018

zkochan left a comment

zkochan Jan 16, 2018

zkochan Jan 16, 2018

zkochan Jan 16, 2018

zkochan Jan 16, 2018

zkochan Jan 18, 2018 •

edited

Loading

zkochan Jan 18, 2018

zkochan Jan 18, 2018 •

edited

Loading

etamponi Jan 18, 2018

zkochan Jan 19, 2018

zkochan Jan 19, 2018

zkochan Jan 19, 2018

zkochan Jan 19, 2018 •

edited

Loading

etamponi Jan 19, 2018

zkochan Jan 19, 2018

zkochan Jan 19, 2018 •

edited

Loading

etamponi Jan 19, 2018

zkochan Jan 20, 2018

etamponi Jan 20, 2018 •

edited

Loading

etamponi commented Jan 21, 2018

zkochan commented Jan 21, 2018

etamponi commented Jan 21, 2018

etamponi commented Jan 21, 2018

zkochan commented Jan 21, 2018

zkochan left a comment

zkochan Jan 21, 2018

zkochan commented Jan 21, 2018

zkochan commented Jan 22, 2018

		@@ -385,6 +385,7 @@ async function install (

		const peerDependencies = peerDependenciesWithoutOwn(pkg)

		const nodeMajor = process.version.substring(0, process.version.indexOf('.'))

side effects cache #38

side effects cache #38

Conversation

etamponi commented Jan 16, 2018

zkochan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etamponi Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

etamponi commented Jan 21, 2018

zkochan commented Jan 21, 2018

etamponi commented Jan 21, 2018

etamponi commented Jan 21, 2018

zkochan commented Jan 21, 2018

zkochan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan commented Jan 21, 2018

zkochan commented Jan 22, 2018

zkochan Jan 18, 2018 •

edited

Loading

zkochan Jan 18, 2018 •

edited

Loading

zkochan Jan 19, 2018 •

edited

Loading

zkochan Jan 19, 2018 •

edited

Loading

etamponi Jan 20, 2018 •

edited

Loading