Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump node-sass to v8 #316

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Bump node-sass to v8 #316

wants to merge 3 commits into from

Conversation

StorytellerCZ
Copy link
Member

@StorytellerCZ StorytellerCZ commented Aug 8, 2023

Release as v4.16

A small update before #296

@Zegnat
Copy link

Zegnat commented Sep 4, 2023

I tried running with this patch locally, but Meteor started complaining about Babel 🤔 Have not had the time to look into this more, but thought I would leave a note here in case someone else runs into it. Might need a little more configuration before this can be merged and released?

=> Started HMR server.
=> Errors prevented startup:

   While loading plugin `compileScssBatch` from package `fourseven:scss`:
   packages/babel-runtime.js:20:9:
   The @babel/runtime npm package could not be found in your node_modules
   directory. Please run the following command to install it:

   meteor npm install --save @babel/runtime

   at module (packages/babel-runtime.js:20:9)
   at fileEvaluate (packages/modules-runtime.js:336:7)
   at Module.require (packages/modules-runtime.js:238:14)
   at require (packages/modules-runtime.js:258:21)
   at packages/babel-runtime.js:53:15
   at packages/babel-runtime.js:58:3


=> Your application has errors. Waiting for file change.

@StorytellerCZ
Copy link
Member Author

Interesting. @babel/runtime though is a package that I have installed in all my projects. I think it is also the default in the Meteor templates. I will have to look more into this.

@ebroder
Copy link
Contributor

ebroder commented Oct 31, 2023

I ran into the same issue when I was trying to follow up from #317. It's pretty easy to reproduce:

evan@mathias src % meteor create --minimal meteor-test
[...]
evan@mathias src % cd meteor-test
evan@mathias meteor-test % mkdir packages
evan@mathias meteor-test % git clone -b release/4.16 git@github.com:Meteor-Community-Packages/meteor-scss packages/fourseven_scss
[...]
evan@mathias meteor-test % meteor add fourseven:scss
compileScssBatch: updating npm dependencies -- node-sass...
 => Errors while adding packages:

While loading plugin `compileScssBatch` from package `fourseven:scss`:
packages/babel-runtime.js:20:9:
The @babel/runtime npm package could not be found in your node_modules
directory. Please run the following command to install it:

meteor npm install --save @babel/runtime

at module (packages/babel-runtime.js:20:9)
at fileEvaluate (packages/modules-runtime.js:336:7)
at Module.require (packages/modules-runtime.js:238:14)
at require (packages/modules-runtime.js:258:21)
at packages/babel-runtime.js:53:15
at packages/babel-runtime.js:58:3

Whatever is happening, installing @babel/runtime isn't sufficient to fix it:

evan@mathias meteor-test % meteor npm i --save @babel/runtime
[...]
evan@mathias meteor-test % meteor add fourseven:scss
 => Errors while adding packages:

While loading plugin `compileScssBatch` from package `fourseven:scss`:
packages/babel-runtime.js:20:9:
The @babel/runtime npm package could not be found in your node_modules
directory. Please run the following command to install it:

meteor npm install --save @babel/runtime

@wreiske
Copy link

wreiske commented Dec 13, 2023

I was successfully able to bump node-sass to 7.0.3, but anything above it causes an issue with the lru-cache.

=> Started proxy.                             
=> Started HMR server.                        
=> Errors prevented startup:                  
   
   While running registerCompiler callback in package fourseven:scss:
   /home/debian/removed/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 (/home/debian/removed/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:43:5)
   at packages/compileScssBatch/plugin/compile-scss.js:14:10

To get it working I did:

Package.registerBuildPlugin({
  name: 'compileScssBatch',
  use: ['caching-compiler@1.2.2 || 2.0.0', 'ecmascript@0.16.7'],
  sources: ['plugin/compile-scss.js'],
  npmDependencies: {
    'node-sass': '7.0.3',
    '@babel/runtime': '7.23.6',
  },
});

I'm still running into an issue though (which is why I started this upgrade rabit trail...):

error: Scss compiler error: Error: Function rgb is missing argument $green.
   on line 1 of {}/imports/ui/clients/monitors/_monitors.scss
   from line 32 of {}/client/main.scss
   >> .mapboxgl-map{-webkit-tap-highlight-color:rgb(0 0 0/0);font:12px/20px Helvet
   ------------------------------------------^

I think it's related to node-scss being so old, see https://stackoverflow.com/a/71806296

May need to mess with the caching-compiler to get this working fully on versions above 7.0.3. https://github.com/meteor/meteor/tree/master/packages/caching-compiler

@StorytellerCZ
Copy link
Member Author

@wreiske does v6 work without the issue? At the very least lets update to the latest version that doesn't give us trouble and then we can investigate #296

@wreiske
Copy link

wreiske commented Dec 13, 2023

@wreiske does v6 work without the issue? At the very least lets update to the latest version that doesn't give us trouble and then we can investigate #296

My code above is working for me in production. 7.0.3.

The issue I am having is that bumping to 7.0.3 didn't fix an issue I was having with the OLD version of node-sass... When I try to import mapbox everything explodes (see the stack overflow link on my last post).

I have not ran the package tests, but 7.0.3 with the added babel works for me! I would recommend more testing before calling it good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants