Dependency question: node-sass

0dp commented 4 years ago

Why are stencil-cli/stencil-styles using its own fork of node-sass that seems to be of a vintage version?

https://github.com/bigcommerce-labs/node-sass

carsonreinke commented 4 years ago

@0dp it is odd, especially since it causes a number of issues.

https://github.com/bigcommerce/stencil-cli/issues/581#issuecomment-595441327

0dp commented 4 years ago

Yes very peculiar. It almost seems as a forgotten relic

carsonreinke commented 4 years ago

...and the reason for only Node 10 support, from my understanding.

junedkazi commented 4 years ago

The reason for us to maintain a node-sass fork is because a while back node sass a breaking change and to protect the themes on the platform we had to fork. See the links below for more details;

https://github.com/sass/node-sass/issues/1472 https://medium.com/@xzyfer/why-node-sass-broke-your-code-and-semver-1b3e409c57b9

We are ideating on a solution where we can be backward compatible but switch over to https://github.com/sass/node-sass but it is going to be a while before we can do that.

0dp commented 4 years ago

I totally understand where BC is coming from. But to me at least, it also seems that when node-sass 'fixed' that bug, they were unable to foresee the consequences. Hence I can imagine it resulted in some hasty desissions being employed by some projects relying on said bug. Which now, 4 years later have created technical debt for some of these projects.

I personally only found out when I was trying to use some sass that was not supported by the version in use.

But to be a little constructive.

Have bc made an quantitative analysis of how widely employed this 'bug' is being used in your eco system?
What strategies for updating, and maintaining some backward compatibility have been discussed?
Have bc considered sending out, or have bc previously sent out at press release to theme authors, not to rely on this bug going forward? - Or is it in fact the internal tools used by BC that needs to adhere to the changes imposed by that fix?
Is there anything the community can do?

carsonreinke commented 4 years ago

Is there anything the community can do?

@0dp I think that moving away from server rendered Sass would be the best idea. This will allow the theme builder to manage that instead of having this on BigCommerce shoulders.

LordZardeck commented 2 years ago

This could be mitigated by just using the sass project (Dart Sass) which is recommended by node-sass, which is also deprecated now.

0dp commented 2 years ago

It looks like there has been some work on it over in @bigcommerce/stencil-styles

    activateEngine(engine) {
        if (engine === 'node-sass') {
            this.compilers['scss'].activateNodeSassEngine();
        } else if (engine === 'node-sass-fork') {
            this.compilers['scss'].activateNodeSassForkEngine();
        } else if (engine === 'dart-sass') {
            this.compilers['scss'].activateDartSassEngine();
        }
    }

But i can't find anywhere it actually does anything.

In your themes config.json suppose you would eventually do something like

  "css_compiler": "scss",
  "engine": "dart-sass",
  "autoprefixer_cascade": true,
  "autoprefixer_browsers": [
    "> 1%",
    "last 2 versions",
    "Firefox ESR"
  ],

Though as you say @LordZardeck it should just be sass and ultimately stencil-styles should be deprecated. Stencil-cil should bud out of the build tasks themselves, and leave that to be contained in the theme.

But I am just spitballing here

andreyvolokitin commented 2 years ago

The reason for us to maintain a node-sass fork is because a while back node sass a breaking change and to protect the themes on the platform we had to fork. See the links below for more details;

sass/node-sass#1472 https://medium.com/@xzyfer/why-node-sass-broke-your-code-and-semver-1b3e409c57b9

We are ideating on a solution where we can be backward compatible but switch over to https://github.com/sass/node-sass but it is going to be a while before we can do that.

@junedkazi we now have dart-sass in stencil-cli by default since v3.6.0, so it is active during dev mode. But looks like a production server-side compilation (when a theme pushed into bigcommerce i.e. via stencil push) still uses deprecated compilator. I encountered it when trying to compile Bootstrap v5.2 — the output CSS is broken using server-side compilation (as well as pre-v3.6.0 stencil-cli compilation), but is working well in stencil-cli@v3.6.0 and above. And it's most likely not the minifier in charge for this, because the selectors themselves differ in CSS output, i.e. .a.b can turn into .a .b.

Server-side compilation is supposedly kept in sync with stencil-cli, but looks like it's not the case anymore. Does stencil-cli and server-side compiler are indeed the same currently? If not, is it possible to introduce a way to set a desirable server-side compiler for a store (i.e. "legacy" or "modern")? @jairo-bc @mcampa

andreyvolokitin commented 2 years ago

I investigated this further.

The test SCSS:

@mixin form-validation-state-selector($state) {
  @if ($state == "valid" or $state == "invalid") {
    .was-validated #{if(&, "&", "")}:#{$state},
    #{if(&, "&", "")}.is-#{$state} {
      @content;
    }
  } @else {
    #{if(&, "&", "")}.is-#{$state} {
      @content;
    }
  }
}

.a {
    @include form-validation-state-selector('valid') {
        color: red;
    }
}

The correct output:

.was-validated .a:valid, .a.is-valid {
  color: red;
}

The wrong output produced by node-sass fork:

.a .was-validated :valid,
.a .is-valid {
  color: red; }

dart sass and deprecated node-sass both produce correct CSS (can be tested at https://www.sassmeister.com/)
node-sass fork produces incorrect CSS
server-side BC compilation is out of sync with current stencil-cli developments (it uses a node-sass fork and produces incorrect CSS output)

@jairo-bc is it still required for server-side BigCommerce SCSS compilation to use a node-sass fork as stated here? Can this be solved by introducing an option for a storefront to choose a SASS compilator? I also think it could be best to just use dart-sass on both ends

I created related issue for stencil-cli: https://github.com/bigcommerce/stencil-cli/issues/995

jairo-bc commented 2 years ago

@andreyvolokitin Thank you for the detailed comments! Stencil cli by default is using latest node-sass, which is still maintained. We are still moving from the fork towards the latest node sass version.

Looks like that is some breaking change between the versions, I would recommend to use syntax is supported by both versions.

bc-0dp commented 2 years ago

@andreyvolokitin Thank you for the detailed comments! Stencil cli by default is using latest node-sass, which is still maintained.

Maintained but not with features or compatibility with any new CSS or Sass features 😭

bigcommerce / stencil-cli

Dependency question: node-sass #600