meteor/meteor

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

Opened this issue · 10 comments

perbergland commented

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 commented

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 commented

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 commented

@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 commented

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

perbergland commented

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 commented

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 commented

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 commented

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: ["caching-compiler@1.2.2 || 2.0.0-rc300.1", "ecmascript@0.15.1"],