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

SassBoolean should be a class #358

Closed
nex3 opened this issue Jan 22, 2025 · 4 comments · Fixed by #359
Closed

SassBoolean should be a class #358

nex3 opened this issue Jan 22, 2025 · 4 comments · Fixed by #359
Assignees
Labels
bug Something isn't working

Comments

@nex3
Copy link
Contributor

nex3 commented Jan 22, 2025

Hello,

I'm trying to migrate a script from sass to sass-embedded and can't use SassBoolean anymore.

The script tries to identify the type with:

if (value instanceof SassBoolean) {

  (...)

}

At runtime, the following error is thrown:

Error: TypeError: Right-hand side of 'instanceof' is not an object

I realized SassBoolean is an interface and not a class:

export interface SassBoolean extends Value {

Therefore, it is not present here in the compiled index.js along sassFalse and sassTrue:

var boolean_2 = require("./src/value/boolean");

Object.defineProperty(exports, "sassFalse", { enumerable: true, get: function () { return boolean_2.sassFalse; } });

Object.defineProperty(exports, "sassTrue", { enumerable: true, get: function () { return boolean_2.sassTrue; } });

Should one use sassFalse and sassTrue instead ?

As is, SassBoolean still breaks usage of sass-embedded as drop-in replacement, as mentioned by @rvock.

Originally posted by @guillaumerochelle in #337

The spec defines SassBoolean as a class, so this is indeed a bug in the Embedded Host's implementation of the Sass API.

@Goodwine
Copy link
Member

I'm reading through the code, it seems that SassBoolean was left as an interface so that it could not be called with weird things like new SassBoolean(123 as boolean).

To preserve that intent, I think we can simply replace the interface with an abstract class, and then the use case posted by @guillaumerochelle should work, I think

@nex3
Copy link
Contributor Author

nex3 commented Jan 28, 2025

The actual typescript annotations that clients see are the ones from the spec directory, so there's no type-checking value in having the implementation have different types.

@Goodwine
Copy link
Member

Right, this PR should fix that - #359

@guillaumerochelle
Copy link

Thanks @Goodwine and @nex3 ! Can't wait to patch that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants