Meteor3 - MultiFileCachingCompiler does not set up its internal LRU cache correctly

[perbergland]

Metor 3 RC 0

When trying to use minimal effort to port over fourseven:scss for meteor 3 (not moving to dart), I got the following runtime error during compilation:

While running registerCompiler callback in package fourseven:scss:

/XXX/packages/meteor-scss/.npm/plugin/compileScssBatch/node_modules/lru-cache/index.js:196:15:
cannot set sizeCalculation without setting maxSize or maxEntrySize
at new LRUCache
(/XXX/packages/meteor-scss/.npm/plugin/compileScssBatch/node_modules/lru-cache/index.js:196:15)
at new MultiFileCachingCompiler
(packages/caching-compiler/multi-file-caching-compiler.js:26:19)
at new SassCompiler (packages/compileScssBatch/plugin/compile-scss.js:45:5)

This is probably caused by some breaking change in lru-cache; caching-compiler/package.js does not specify the version to bring in.

[perbergland]

maybe related to https://github.com/isaacs/node-lru-cache/issues/221 ?

[perbergland]

Right, so length is a deprecated name for an option for lru-cache and has been renamed to sizeCalculation, which requires maxSize or maxEntrySize. This is for lru-cache version 7.18.3 which is what fourseven:scss brings in. The bug is in MultiFileCachingCompiler though since it doesn’t specify the version of lru-cache to use, it doesn’t even list lru-cache in its package.js.

[perbergland]

see https://github.com/isaacs/node-lru-cache/blob/1e07c5e70efe8a69cf4f63b9aeb0813fc1615f6a/index.js#L193

[perbergland]

Changing the LRU constructor call to below fixes the immediate problem. No idea if it’s the correct value for maxSize. Also, length should be renamed sizeCalculation but I do not understand which version of lru-cache that is supposed to be used in this context.

    this._cache = new LRU({
      max: this._cacheSize,
      maxSize: this._cacheSize,
      // We ignore the size of cacheKeys here.
      length: (value) => this.compileResultSize(value.compileResult),
    });

[harryadel]

@perbergland Thank you for starting this issue. I'm running into the same problem. activitytree fork isn't the best route forward as it causes errors with meteor imports @import '{meteoric124:meteoric-sass}/scss/ionic'; so when I tried to update it to use the latest version of node-sass I ran into the lru-cache issue illustrated here. I guess I'll attempt to fork caching-compiler locally to temporarily fix this problem.

@nachocodoner @henriquealbert Can this be added to the next RC?

~~EDIT: length is deprecated, sizeCalculation is to be used instead.~~

[harryadel]

I think the bigger question here is did lru-cache get updated?? If so, what're the implications on other packages/modules?

[perbergland]

I have not quite grasped how meteor specifies and locks down npm package versions.

Here’s a documentation link to when it was updated to 4.1.5 in 2.3 but I cannot find any code to match that dependency.

Then there are transitive dependencies, e.g. via semver which was updated in fourseven:scss and via some internal script

lru-cache is also brought in via @babel

[perbergland]

The top level lock file in release-3.0 has both 5.1.1 and 6.0.0 in it

https://github.com/meteor/meteor/blob/eee47ae1d9aebd4c4543249f05faabfa23b1d122/package-lock.json

[nachocodoner]

Do you use the meteor/scss version from this PR?

I wonder if the upgrade there on sass produced lru-cache to upgrade there and then affected the other package. Anyway, we may look into it. Could you provide an exact reproduction repository to serve us as a guidence on replicate the problem and find a fix?

I see lru-cache being used in many places on Meteor core packages, which any change on the version and the breaking changes may affect badly. So I would like to understand better the issue and where it comes.

[perbergland]

I used the master branch of https://github.com/Meteor-Community-Packages/meteor-scss/blob/master/package.js and just put the repo into my ./packages folder and just modified a dependency:

  use: ["[email protected] || 2.0.0-rc300.1", "[email protected]"],

[StorytellerCZ]

Closing this since the relevant PR has been merged.