oddbird / true

Sass unit tests
http://oddbird.net/true/
BSD 3-Clause "New" or "Revised" License
683 stars 46 forks source link

Auto-detect `sass` vs `sass-embedded` #290

Open JoostKersjes opened 1 day ago

JoostKersjes commented 1 day ago

I'm using "sass-true": "8.0.0" and incorrectly assumed that it would work with "sass-embedded": "1.79.2".

The reason for replacing sass with sass-embedded is mostly for improved performance.

Error message:

 FAIL  test/sass.test.js [ test/sass.test.js ]
Error: Cannot find Dart Sass (`sass`) dependency.
 ❯ Object.runSass ../node_modules/.pnpm/sass-true@8.0.0/node_modules/sass-true/src/index.ts:112:13
 ❯ test/sass.test.js:13:44
     11|   });
     12| 
     13|   sassTestFiles.forEach((file) => sassTrue.runSass({ describe, it }, file));
       |                                            ^
     14| });
     15| 
 ❯ test/sass.test.js:13:17
JoostKersjes commented 1 day ago

The pipelines on our project did not fail for the change to replace sass with sass-embedded. I'm not exactly sure yet why they only started failing a day after the changes were merged, but that alone makes me think this is to do with caching.

JoostKersjes commented 1 day ago

Sorry for creating an issue in a rush. It turns out that this problem can be avoided by simply passing the sass-embedded as an option to True like such:

import { dirname, resolve } from "node:path";
import { fileURLToPath } from "node:url";
import * as sass from "sass-embedded";
import sassTrue from "sass-true";
import { describe, it } from "vitest";

describe("Sass", () => {
  const sassTestFiles = Object.keys(
    import.meta.glob("./**/*.test.scss", { eager: true }),
  ).map((relativePath) => {
    return resolve(dirname(fileURLToPath(import.meta.url)), relativePath);
  });

  sassTestFiles.forEach((file) =>
    sassTrue.runSass({ describe, it, sass }, file),
  );
});

I think it would still be beneficial if sass-true could auto-detect if it should grab the sass or sass-embedded package, but this option workaround relieves any urgency.

jgerigmeyer commented 17 hours ago

@JoostKersjes I think that's a good idea -- I've renamed this issue.