implement first class mixins #2073

connorskees · 2023-08-25T20:59:35Z

Tracking issue: sass/sass#626
Protobuf changes: sass/sass#3674
Spec tests: sass/sass-spec#1933
Embedded host: sass/embedded-host-node#253

Closes #2046

lib/src/callable/built_in.dart

lib/src/functions/meta.dart

connorskees · 2023-08-28T21:13:22Z

I'm not able to reproduce the CI build failures locally. I assume it's to do with missing auto-generated (and gitignored) files that I have locally but which aren't created during the CI run. Is there something I'm missing updating for the embedded protocol files?

ntkme · 2023-08-29T16:13:04Z

@connorskees If you are making changes to the protobuf definition, you need to submit a PR for https://github.com/sass/sass, and link the PR in the description of this PR.

connorskees · 2023-09-02T03:20:19Z

I've reverted the embedded protocol changes for now as adding the protobuf changes to sass/sass and linking the issues didn't seem to resolve the build failures.

nex3 · 2023-09-06T00:07:15Z

It looks like the CI is picking up the protocol buffer definition changes (at least now), so you may want to roll forward that patch.

lib/src/callable/built_in.dart

lib/src/functions/meta.dart

lib/src/value.dart

lib/src/visitor/async_evaluate.dart

This reverts commit f715c22.

lib/src/embedded/mixin_registry.dart

lib/src/value/mixin.dart

lib/src/visitor/async_evaluate.dart

lib/src/functions/meta.dart

lib/src/visitor/async_evaluate.dart

lib/src/callable/async_built_in.dart

lib/src/visitor/async_evaluate.dart

lib/src/embedded/opaque_registry.dart

lib/src/value/mixin.dart

lib/src/visitor/async_evaluate.dart

…rst-class-mixins

connorskees · 2023-10-03T23:25:25Z

Do I need to link to the embedded host PR in this PR description?

nex3 · 2023-10-03T23:26:59Z

Yes, you do.

connorskees · 2023-10-03T23:28:10Z

Updated.

ntkme · 2023-10-03T23:30:01Z

Updated.

You will also need to push a different commit in order for CI to get the updated PR description. E.g. an empty created by git commit --allow-empty would work.

…rst-class-mixins

ntkme · 2023-10-04T00:43:04Z

lib/src/js/value/mixin.dart

+
+/// The JavaScript `SassMixin` class.
+final JSClass mixinClass = () {
+  var jsClass = createJSClass('sass.SassMixin', () {


Not sure if it is related to the browser failure, but I think the argument is missing Object self here when looking at all other values.

connorskees · 2023-10-04T00:47:40Z

I added back the assertion to the sass-spec to see if adding that parameter fixes the browser failure.

ntkme · 2023-10-04T00:57:07Z

lib/src/js/value/mixin.dart

+  getJSClass(SassMixin(Callable('f', '', (_) => sassNull)))
+      .injectSuperclass(jsClass);


Also this does not make much sense. If I understand it correctly the reason we have this for Sass Function type is to allow the "JS" type to inherit the prototype of dart-converted-JS type? If so here the argument passed into SassMixin (the dart type) should be some argument necessary to construct an actual Mixin object.

@nex3 I don't fully understand how the JS-interop works so please correct me if I'm wrong.

Never mind. Looks like Mixin just takes AsyncCallable so this is probably fine.

connorskees · 2023-10-04T05:09:38Z

If possible, I would like for the first class mixin PRs to merge quite soon -- after this week I will have significantly less time to work outside of work.

nex3 · 2023-10-04T20:39:09Z

It's not clear to me why the instanceof check isn't working in a browser environment. Injecting the superclass should make it respond to instanceof. You might need to load it up yourself in a browser and poke around there to see where the disconnect is between what should be happening and what is.

connorskees · 2023-10-04T20:41:50Z

Seems it might be a known jest issue? See jestjs/jest#5946 and jestjs/jest#2549 and similar issues on stackoverflow, https://stackoverflow.com/questions/52551035/tobeinstanceofnumber-does-not-work-in-jest, https://stackoverflow.com/questions/58029714/jests-expectvalue-tobeinstanceofclass-fails-for-expectutil-promisify, and others.

nex3 · 2023-10-04T23:24:21Z

Jest does have weird instanceof errors, but we don't use Jest in the browser. That also wouldn't explain why the instanceof check fails for SassMixin but succeeds for other Value subtypes.

connorskees · 2023-10-05T00:05:36Z

The most recent error is interesting: Error: Right-hand side of 'instanceof' is not an object. This would imply to me SassMixin imported from 'sass' in the browser is undefined (or something else??).

ntkme · 2023-10-05T00:15:50Z

@connorskees I think I found the root cause: we're missing ESM export here:

dart-sass/tool/grind.dart

Lines 49 to 65 in 16b8512

    
           pkg.jsEsmExports.value = { 
        
             'compile', 
        
             'compileAsync', 
        
             'compileString', 
        
             'compileStringAsync', 
        
             'Logger', 
        
             'SassArgumentList', 
        
             'SassBoolean', 
        
             'SassCalculation', 
        
             'CalculationOperation', 
        
             'CalculationInterpolation', 
        
             'SassColor', 
        
             'SassFunction', 
        
             'SassList', 
        
             'SassMap', 
        
             'SassNumber', 
        
             'SassString',

Node runs the CJS entrypoint but browser runs the ESM entrypoint...

connorskees · 2023-10-05T00:20:34Z

Nice find! Thank you -- that would have been annoying to find otherwise.

nex3 · 2023-10-05T00:25:57Z

I wonder if it would be possible to add a conformance check for that...

nex3 · 2023-10-05T01:11:13Z

All right, looks like everything is passing but sass/embedded-host-node#253. Once sass/embedded-host-node#255 lands, I'll merge these all in and we should be good to go. Thanks for the hard work, @connorskees!

* feature.color-4: (30 commits) Update for `lch()` tests (sass#2108) Add support for relative color syntax (sass#2112) Update for `oklab()` tests (sass#2094) Poke CI Cut a release (sass#2107) Implement first class mixins (sass#2073) Fix a race condition preventing embedded compiler to shutdown after a protocol error (sass#2106) Switch to the GitHub-hosted MacOS ARM64 runner (sass#2103) Update the version of Sass used by the website on release (sass#2102) Bump actions/checkout from 3 to 4 (sass#2088) Bump docker/setup-qemu-action from 2 to 3 (sass#2089) Implement support for the relative color syntax of CSS Color 5 (sass#2098) Rephrase errors for numbers that must be unitless or % (sass#2101) Forbid LLM contributions (sass#2100) Update protocol-version during embedded-host-node release (sass#2097) Deprecate Deprecation.calcInterp (sass#2096) Avoid useless allocations for interpolations without maps (sass#2095) Fix an error during embedded compiler shutdown (sass#2092) Cut a release (sass#2090) Expose the containing URL to importers under some circumstances (sass#2083) ...

stof · 2024-04-01T12:59:10Z

lib/src/visitor/async_evaluate.dart

@@ -418,6 +418,19 @@ final class _EvaluateVisitor
        });
      }, url: "sass:meta"),

+      BuiltInCallable.function("module-mixins", r"$module", (arguments) {


@nex3 is it expected that those functions are exposed as global aliases as well and not just as module functions or is it a mistake ? The spec PR does not mention those aliases).

The metaFunctions variable defined here is not split between local and global lists (probably because when it was introduced, there were no local-only functions).

Note that metaMixins below is local-only (there is no global aliases for those mixins)

No, they aren't expected to be exposed globally.

connorskees added 3 commits August 25, 2023 20:55

initial impl of first class mixins

ef23769

add mixin type to js api

0b9f3a8

add first class mixins to embedded protocol

63c1f5e

connorskees marked this pull request as ready for review August 26, 2023 04:07

connorskees added 2 commits August 26, 2023 04:19

support AsyncBuiltInCallable in accepts-content

f9badfa

generate evaluate file

010ad35

connorskees commented Aug 26, 2023

View reviewed changes

lib/src/callable/built_in.dart Outdated Show resolved Hide resolved

lib/src/functions/meta.dart Outdated Show resolved Hide resolved

jathak requested a review from nex3 August 28, 2023 20:47

connorskees mentioned this pull request Aug 29, 2023

add first class mixins to embedded sass protobuf and JS API sass/sass#3674

Merged

connorskees added 3 commits August 29, 2023 19:31

bump ci

b908e27

merge main

7d6757c

revert embedded protocol changes

f715c22

Style nits

c0eced3

nex3 requested changes Sep 6, 2023

View reviewed changes

connorskees added 5 commits September 7, 2023 14:56

some pr review

91ac279

Revert "revert embedded protocol changes"

194e171

This reverts commit f715c22.

remove redundant content cast

154e12e

correctly pass content through meta.apply

ca57cb0

correctly pass through acceptsContent value

4093914

connorskees requested a review from nex3 September 8, 2023 20:42

nex3 requested changes Sep 13, 2023

View reviewed changes

connorskees added 4 commits September 14, 2023 17:48

pr review

eb33c56

refactor embedded registry

eeb14ee

merge main

1cfd14b

merge main

5e1774c

connorskees requested a review from nex3 September 18, 2023 21:28

connorskees mentioned this pull request Sep 29, 2023

add first class mixins sass/embedded-host-node#253

Merged

stof reviewed Sep 29, 2023

View reviewed changes

lib/src/embedded/opaque_registry.dart Outdated Show resolved Hide resolved

lib/src/embedded/opaque_registry.dart Outdated Show resolved Hide resolved

lib/src/value/mixin.dart Outdated Show resolved Hide resolved

lib/src/visitor/async_evaluate.dart Outdated Show resolved Hide resolved

connorskees added 3 commits September 30, 2023 16:12

nits

3144e02

lazily evaluate file spans

f1c1d5a

Merge branch 'main' of https://github.com/sass/dart-sass into feat/fi…

895b2ca

…rst-class-mixins

connorskees added 3 commits October 3, 2023 23:30

bump ci

96bbca2

bump ci

679e1bd

Merge branch 'main' of https://github.com/sass/dart-sass into feat/fi…

65f2d2d

…rst-class-mixins

ntkme reviewed Oct 4, 2023

View reviewed changes

take Object self in JS constructor

75ccdfb

ntkme reviewed Oct 4, 2023

View reviewed changes

bump ci

6277d5d

add SassMixin to ESM export

f1b4ff3

nex3 merged commit ce545c2 into sass:main Oct 5, 2023
44 checks passed

stof reviewed Apr 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement first class mixins #2073

implement first class mixins #2073

connorskees commented Aug 25, 2023 •

edited

Loading

connorskees commented Aug 28, 2023

ntkme commented Aug 29, 2023

connorskees commented Sep 2, 2023

nex3 commented Sep 6, 2023

connorskees commented Oct 3, 2023

nex3 commented Oct 3, 2023

connorskees commented Oct 3, 2023

ntkme commented Oct 3, 2023

ntkme Oct 4, 2023

connorskees commented Oct 4, 2023

ntkme Oct 4, 2023 •

edited

Loading

ntkme Oct 4, 2023

connorskees commented Oct 4, 2023

nex3 commented Oct 4, 2023

connorskees commented Oct 4, 2023 •

edited

Loading

nex3 commented Oct 4, 2023

connorskees commented Oct 5, 2023

ntkme commented Oct 5, 2023

connorskees commented Oct 5, 2023

nex3 commented Oct 5, 2023

nex3 commented Oct 5, 2023

stof Apr 1, 2024

stof Apr 1, 2024

nex3 Apr 1, 2024

		getJSClass(SassMixin(Callable('f', '', (_) => sassNull)))
		.injectSuperclass(jsClass);

implement first class mixins #2073

implement first class mixins #2073

Conversation

connorskees commented Aug 25, 2023 • edited Loading

connorskees commented Aug 28, 2023

ntkme commented Aug 29, 2023

connorskees commented Sep 2, 2023

nex3 commented Sep 6, 2023

connorskees commented Oct 3, 2023

nex3 commented Oct 3, 2023

connorskees commented Oct 3, 2023

ntkme commented Oct 3, 2023

ntkme Oct 4, 2023

Choose a reason for hiding this comment

connorskees commented Oct 4, 2023

ntkme Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

ntkme Oct 4, 2023

Choose a reason for hiding this comment

connorskees commented Oct 4, 2023

nex3 commented Oct 4, 2023

connorskees commented Oct 4, 2023 • edited Loading

nex3 commented Oct 4, 2023

connorskees commented Oct 5, 2023

ntkme commented Oct 5, 2023

connorskees commented Oct 5, 2023

nex3 commented Oct 5, 2023

nex3 commented Oct 5, 2023

stof Apr 1, 2024

Choose a reason for hiding this comment

stof Apr 1, 2024

Choose a reason for hiding this comment

nex3 Apr 1, 2024

Choose a reason for hiding this comment

connorskees commented Aug 25, 2023 •

edited

Loading

ntkme Oct 4, 2023 •

edited

Loading

connorskees commented Oct 4, 2023 •

edited

Loading