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

Remove grunt-sass and node-sass as dependencies #638

Closed
5 tasks
taoeffect opened this issue Aug 27, 2019 · 1 comment · Fixed by #755
Closed
5 tasks

Remove grunt-sass and node-sass as dependencies #638

taoeffect opened this issue Aug 27, 2019 · 1 comment · Fixed by #755
Labels
App:Frontend Kind:Enhancement Improvements, new features, performance upgrades, etc. Note:Tooling

Comments

@taoeffect
Copy link
Member

taoeffect commented Aug 27, 2019

Problem

  1. Having node-sass as a dependency makes it difficult to develop with Docker because it downloads a linux binary on macOS
  2. It's an unnecessary dependency because rollup-plugin-vue already uses sass to compile sass/scss.

Solution

  • In index.html, remove /assets/css/main.css
  • Remove grunt-sass and node-sass usage from .Gruntfile.babel.js
  • import the main.scss stylesheet in main.js so that it's processed by rollup-plugin-vue
  • verify that Docker support has now improved
  • remove node-sass and grunt-sass from package.json

Related: #372

@taoeffect taoeffect self-assigned this Aug 27, 2019
@taoeffect taoeffect added App:Frontend Kind:Enhancement Improvements, new features, performance upgrades, etc. labels Aug 27, 2019
@taoeffect
Copy link
Member Author

taoeffect commented Aug 28, 2019

Unfortunately, while attempting this, I ran into two issues:

  1. (low severity) main.css.map is no longer generated because node-sass is no longer being used, and rollup-plugin-vue wont' generate a corresponding component.css.map
  2. (high severity) I couldn't get rollup to watch the .scss files in frontend/assets/style for changes, and so couldn't get it to regenerate the dist/js/main.js file (which causes grunt-contrib-watch to trigger the browser refresh)

So it looks like we're stuck with it for now. Have a look at rollup-awesome for potential solutions. Here's a diff that shows my attempt:

diff --git a/.Gruntfile.babel.js b/.Gruntfile.babel.js
index cefc4b1..21fa088 100644
--- a/.Gruntfile.babel.js
+++ b/.Gruntfile.babel.js
@@ -52,11 +52,11 @@ module.exports = (grunt) => {
         options: { livereload },
         files: [`${distJS}/main.js`]
       },
-      css: {
-        options: { livereload },
-        files: ['frontend/assets/**/*.{sass,scss}'],
-        tasks: ['sass']
-      },
+      // css: {
+      //   options: { livereload },
+      //   files: ['frontend/assets/**/*.{sass,scss}'],
+      //   tasks: ['sass']
+      // },
       html: {
         options: { livereload },
         files: ['frontend/**/*.html'],
@@ -72,25 +72,25 @@ module.exports = (grunt) => {
       }
     },
 
-    sass: {
-      options: {
-        implementation: require('node-sass'),
-        sourceMap: development,
-        // https://github.com/vuejs/vueify/issues/34#issuecomment-161722961
-        // indentedSyntax: true,
-        // sourceMapRoot: '/',
-        outputStyle: development ? 'nested' : 'compressed'
-      },
-      dev: {
-        files: [{
-          expand: true,
-          cwd: 'frontend/assets/style',
-          src: ['*.{sass,scss}', '!_*/**'],
-          dest: distCSS,
-          ext: '.css'
-        }]
-      }
-    },
+    // sass: {
+    //   options: {
+    //     implementation: require('node-sass'),
+    //     sourceMap: development,
+    //     // https://github.com/vuejs/vueify/issues/34#issuecomment-161722961
+    //     // indentedSyntax: true,
+    //     // sourceMapRoot: '/',
+    //     outputStyle: development ? 'nested' : 'compressed'
+    //   },
+    //   dev: {
+    //     files: [{
+    //       expand: true,
+    //       cwd: 'frontend/assets/style',
+    //       src: ['*.{sass,scss}', '!_*/**'],
+    //       dest: distCSS,
+    //       ext: '.css'
+    //     }]
+    //   }
+    // },
 
     copy: {
       node_modules: {
@@ -187,7 +187,7 @@ module.exports = (grunt) => {
 
   grunt.registerTask('build', function () {
     const rollup = this.flags.watch ? 'rollup:watch' : 'rollup'
-    grunt.task.run(['exec:eslint', 'exec:puglint', 'exec:stylelint', 'copy', 'sass', rollup])
+    grunt.task.run(['exec:eslint', 'exec:puglint', 'exec:stylelint', 'copy', rollup])
   })
 
   grunt.registerTask('cypress', function () {
@@ -312,6 +312,10 @@ module.exports = (grunt) => {
       moduleContext: {
         'frontend/controller/utils/primus.js': 'window'
       },
+      watch: {
+        exclude: 'node_modules/**',
+        include: 'frontend/assets/styles/**/*.scss'
+      },
       plugins: [
         alias({
           // https://vuejs.org/v2/guide/installation.html#Standalone-vs-Runtime-only-Build
@@ -354,11 +358,16 @@ module.exports = (grunt) => {
           style: {
             preprocessOptions: {
               scss: {
+                // https://github.com/sass/dart-sass#javascript-api
+                sourceMap: development,
+                outputStyle: development ? 'expanded' : 'compressed',
                 // https://github.com/sass/node-sass#includepaths
                 includePaths: [
-                  // so that you can write things like this inside component style sections:
+                  // so that we can write things like this inside component style sections:
                   // @import 'vue-slider-component/lib/theme/default.scss';
-                  path.resolve('./node_modules')
+                  path.resolve('./node_modules'),
+                  // so that we can write @import 'main.scss'; in AppStyles.vue
+                  path.resolve('./frontend/assets/style')
                 ]
               }
             }
diff --git a/frontend/index.html b/frontend/index.html
index f8041e8..8678e88 100644
--- a/frontend/index.html
+++ b/frontend/index.html
@@ -6,7 +6,7 @@
     <title>Group Income Simple</title>
     <meta name="description" content="">
     <meta name="viewport" content="width=device-width, initial-scale=1">
-    <link rel="stylesheet" href="/assets/css/main.css">
+    <!-- <link rel="stylesheet" href="/assets/css/main.css"> -->
     <link rel="stylesheet" href="/assets/css/component.css">
     <link rel="icon" href="/assets/images/group-income-icon-transparent.png" sizes="32x32">
   </head>
diff --git a/frontend/views/components/AppStyles.vue b/frontend/views/components/AppStyles.vue
index ca19455..4f82426 100644
--- a/frontend/views/components/AppStyles.vue
+++ b/frontend/views/components/AppStyles.vue
@@ -40,3 +40,7 @@ export default {
   }
 }
 </script>
+
+<style lang="scss">
+@import 'main.scss';
+</style>

@taoeffect taoeffect removed their assignment Aug 28, 2019
@taoeffect taoeffect changed the title Remove node-sass as a dependency Remove grunt-sass and node-sass as dependencies Sep 20, 2019
sandrina-p pushed a commit that referenced this issue Nov 2, 2019
…#755)

* continue iterating PayGroup

* Bump dependencies + remove grunt-sass/node-sass (Closes #638) + misc

* move classes out of _colors and into _typography to prevent duplication in component css

* PR review changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App:Frontend Kind:Enhancement Improvements, new features, performance upgrades, etc. Note:Tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant