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

Make it easier to prefer sass-embedded over sass #1180

Open
G-Rath opened this issue Jan 20, 2024 · 2 comments
Open

Make it easier to prefer sass-embedded over sass #1180

G-Rath opened this issue Jan 20, 2024 · 2 comments

Comments

@G-Rath
Copy link

G-Rath commented Jan 20, 2024

Note

This could be a "when sass-embedded support is no longer experimental" modification

Modification Proposal

My understanding is that sass-embedded can be considered like-for-like to sass in terms of features and output i.e. assuming you can run sass-embedded, you can always use it instead of sass and get the exact same compiled output (just faster); and the main reason that sass is a thing is because sass-embedded has more complicated OS requirements being a compiled binary.

Thankfully package.json has a way of expressing dependencies which are desirable but OS restricted and so whose incompatibility with the current OS that is being installed on should not be considered a failure, allowing us to specify sass-embedded as a dependency alongside sass as a fallback i.e.

{
  "name": "my-app",
  "devDependencies": {
    "sass": "^1.69.5",
    "sass-loader": "^13.3.2",
    "webpack": "^5.0.0",
    "webpack-cli": "^4.0.0"
  },
  "optionalDependencies": {
    "sass-embedded": "^1.70.0"
  }
}

However it seems that sass-loader always favors loading sass, so in the above it'll never use sass-embedded by default:

function getDefaultSassImplementation() {
let sassImplPkg = "sass";
try {
require.resolve("sass");
} catch (ignoreError) {
try {
require.resolve("node-sass");
sassImplPkg = "node-sass";
} catch (_ignoreError) {
try {
require.resolve("sass-embedded");
sassImplPkg = "sass-embedded";
} catch (__ignoreError) {
sassImplPkg = "sass";
}
}
}
// eslint-disable-next-line import/no-dynamic-require, global-require
return require(sassImplPkg);
}

Now this is technically documented (though in the context of node-sass) and we can explicitly change this using implementation; however that would require us to have resolving logic in our webpack config across all applications and projects.

I figure sass was preferred back when it was just node-sass because the latter is technically deprecated and is not like-for-like, but maybe it's time to consider revising the default implementation order with the introduction of sass-embedded?

I think there are a few ways this could be done which would all be fine:

  • just change the default order to look for sass-embedded first
  • introduce a new implementation on the lines of prefer-sass-embedded (or a dedicated preferSassEmbedded boolean option)
  • support a comma list of implementations to try first to allow specifying order: sass-embedded,sass (though this is probably a bit much given there's not expected to be yet another sass implementation...)

Expected Behavior / Situation

sass-loader prefers sass-embedded by default, allowing us to manage providing the fastest sass implementation possible for the host, such as with optionalDependencies or installing sass-embedded in a dedicated step during building our docker images, provisioning servers, etc.

Actual Behavior / Situation

sass-loader loads sass by default, ignoring sass-embedded

Please paste the results of npx webpack-cli info here, and mention other relevant information


  System:
    OS: Linux 5.15 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish)
    CPU: (20) x64 13th Gen Intel(R) Core(TM) i7-13700H
    Memory: 12.61 GB / 15.46 GB
  Binaries:
    Node: 18.16.0 - ~/.nodenv/versions/18.16.0/bin/node
    Yarn: 1.22.19 - ~/.nodenv/versions/18.16.0/bin/yarn
    npm: 9.5.1 - ~/.nodenv/versions/18.16.0/bin/npm
  Packages:
    sass-loader: ^13.3.2 => 13.3.2 
    webpack: ^5.0.0 => 5.89.0 
    webpack-cli: ^4.0.0 => 4.10.0 
@alexander-akait
Copy link
Member

sass-embedded is still not very stable and a new API (which we need to use in future) is still under constuction, that is why we prefer sass right now

But I am fine with

support a comma list of implementations to try first to allow specifying order: sass-embedded,sass (though this is probably a bit much given there's not expected to be yet another sass implementation...)

@alexander-akait
Copy link
Member

So feel free to send a PR

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

No branches or pull requests

2 participants