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

Resolved an error that occurs when using with sass 1.60.x #205

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Resolved an error that occurs when using with sass 1.60.x #205

merged 5 commits into from
Jun 12, 2023

Conversation

mkusaka
Copy link
Contributor

@mkusaka mkusaka commented Jun 12, 2023

First of all, thank you for creating this wonderful CLI. I use it all the time and it has been a great help.


Currently, when used with the latest version of sass, which is 1.63.3, an error occurs.

repro: on this branch

❯ git checkout 3d3c8fb8bb2732c8ff293236c31bbe877aa1b14e
...
❯ npm i
...
❯ npm run test

> test
> NODE_OPTIONS="--experimental-vm-modules $NODE_OPTIONS" jest

 FAIL  packages/happy-css-modules/src/transformer/scss-transformer.test.ts
  ● handles sass features

    TypeError: Cannot read properties of undefined (reading 'render')

      12 |   return async (options: LegacyOptions<'async'>) => {
      13 |     return new Promise<LegacyResult>((resolve, reject) => {
    > 14 |       sass.render(options, (exception, result) => {
         |            ^
      15 |         if (exception) reject(exception);
      16 |         else resolve(result!);
      17 |       });

      at render (packages/happy-css-modules/src/transformer/scss-transformer.ts:14:12)
      at packages/happy-css-modules/src/transformer/scss-transformer.ts:13:12
      at Locator.render [as transformer] (packages/happy-css-modules/src/transformer/scss-transformer.ts:27:26)
      at Locator.readCSS (packages/happy-css-modules/src/locator/index.ts:119:20)
      at Locator._load (packages/happy-css-modules/src/locator/index.ts:157:40)
      at Locator.load (packages/happy-css-modules/src/locator/index.ts:143:20)
      at Object.<anonymous> (packages/happy-css-modules/src/transformer/scss-transformer.test.ts:40:18)

....

Test Suites: 5 failed, 14 passed, 19 total
Tests:       9 failed, 1 todo, 100 passed, 110 total
Snapshots:   53 passed, 53 total
Time:        6.714 s, estimated 7 s
Ran all test suites.

This is likely due to breaking changes introduced by sass.

ref. sass/dart-sass#2011

In this PR, we will avoid this by using the default when importing, if it exists, and not using the default if it doesn't.

```
Test Suites: 5 failed, 14 passed, 19 total
Tests:       9 failed, 1 todo, 100 passed, 110 total
Snapshots:   53 passed, 53 total
Time:        6.758 s, estimated 7 s
```
@@ -22,7 +22,8 @@ function promisifySassRender(sass: typeof import('sass')) {
export const createScssTransformer: () => Transformer = () => {
let sass: typeof import('sass');
return async (source, options) => {
sass ??= (await import('sass').catch(handleImportError('sass'))).default;
const importedSass = await import('sass').catch(handleImportError('sass'))
sass ??= importedSass.default ?? importedSass;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I implemented this pattern because I confirmed that simply removing .default would cause it to stop working with versions prior to 1.60.x.

@mizdra mizdra added the Type: Fix Fix bug. label Jun 12, 2023
Copy link
Owner

@mizdra mizdra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for contributing. 👍

@mizdra mizdra merged commit 616b1cf into mizdra:main Jun 12, 2023
@mkusaka mkusaka deleted the fix-error branch June 12, 2023 15:27
@mizdra
Copy link
Owner

mizdra commented Jun 12, 2023

@mkusaka This is shipped in 2.1.2. Please could you try it?

@mkusaka
Copy link
Contributor Author

mkusaka commented Jun 12, 2023

@mizdra

Thank you for notifying me. :)

I've just updated to version 2.1.2 in my workspace and verified that everything is functioning as it should 🎉.

I appreciate your support. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix Fix bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants