grunt(core): Remove Excess Grunt Dependencies #1543
Conversation
|
LGTM |
|
My only other question, is should this line be removed. I don't think it will break anything, but want to be sure. Line 56 in 8b54669 |
|
@codydaig I dont see any reference in the code to that key. But I will leave it up to the contributors to confirm. Please see below references for Grunt still on the latest master after the other pull was merged. |
|
@codydaig Looks like the 'gruntConfig' key you mentioned was used only in the gruntfile.js @ L37, L112, L125 |
| @@ -50,7 +50,7 @@ RUN sudo apt-get install -yq nodejs \ | |||
| && apt-get clean \ | |||
| && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | |||
|
|
|||
| # Install gem sass for grunt-contrib-sass | |||
| # Install gem sass | |||
| RUN gem install sass | |||
| @@ -47,7 +47,7 @@ RUN sudo apt-get install -yq nodejs \ | |||
| && apt-get clean \ | |||
| && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | |||
|
|
|||
| # Install gem sass for grunt-contrib-sass | |||
| # Install gem sass | |||
| RUN gem install sass | |||
There was a problem hiding this comment.
Same here, is this even needed anymore?
There was a problem hiding this comment.
@codydaig I'm not sure. Docker is still aloof to me.
There was a problem hiding this comment.
@mleanos It's not Docker. Do users still have to have sass installed through ruby on their computer for it to work?
There was a problem hiding this comment.
I'm just shooting from the hip here, but I think that was only required for older version of Node (or NPM). < 0.10.x?? I haven't looked into that in a while though.
There was a problem hiding this comment.
From gulp-contrib-sass readme it says installing gem sass is a first step. But on the gulp-sass it doesnt have such requirement. It wraps around node-sass -> libsass -> sass libraries.
There was a problem hiding this comment.
I am on Win10 with no Sass/Ruby installed and I just did a simple scss file for core and gulp compiled it to css and i see the style change.
There was a problem hiding this comment.
When I first started using this project I was on Windows 7 & had Node v0.10.x installed; I was required to install ruby/sass with that setup. Now I'm using Windows 10 & Node v6.6.0. I still have Ruby/SASS installed since I did a OS upgrade, so all my installed software came along. I don't want to uninstall ruby/sass just to confirm that I no longer need it :)
What @sujeethk is saying makes sense, if it's indeed a Node version issue.
@sujeethk Just out of curiosity, what Node version are you running?
There was a problem hiding this comment.
@mleanos I use NVM to switch versions. I checked on v6.3.1 and v4.5.0 and it works fine. Although when you switch versions you would have to rebuild node-sass using npm rebuild node-sass. But everything works fine(including sass compile to css and i see those styles applied) without need for installing ruby or sass.
I am not able to check on v0.12.0 as there is a build error. Honestly we should update node engine to remove support for those old versions.
|
Hey guys, looks good and indeed all the references from @sujeethk should be removed as well with regards to their grunt reference. |
|
Changes have been made. WIth some LGTM, I will squash on merge. |
|
@codydaig can you also remove these: |
|
@lirantal Done |
|
Great. Merge whenever you're ready :) |
@sujeethk Just leftover. I must have missed them in the rebase. Thanks for catching that!