search

sass / embedded-host-node

A Node.js library that will communicate with Embedded Dart Sass using the Embedded Sass protocol
MIT License
165 stars 29 forks source link

add first class mixins #253

Closed connorskees closed 1 year ago

connorskees commented 1 year ago

Tracking issue: https://github.com/sass/sass/issues/626 Protobuf changes: https://github.com/sass/sass/pull/3674 Spec tests: sass/sass-spec#1933 dart-sass changes: sass/dart-sass#2073

Closes https://github.com/sass/embedded-host-node/issues/252

connorskees commented 1 year ago

cc @nex3

connorskees commented 1 year ago

I believe CI was failing because I missed adding a link to the dart-sass changes. That should now be resolved.

connorskees commented 1 year ago

I'm a bit confused about where the CI failure is coming from. Is there a place I'm missing updating? All relevant code touching Value should have an associated assertMixin method, and I'm linking to all relevant PRs in the PR description.

ntkme commented 1 year ago

Maybe we need to add a mixin.d.ts to https://github.com/sass/sass/tree/main/js-api-doc/value ?

connorskees commented 1 year ago

I made the change in https://github.com/sass/sass/pull/3674, but CI fails there I believe because the spec file is in the accepted/ folder and not spec/.

ntkme commented 1 year ago

The spec failure is due to the mismatch between js-api-doc/ and spec/js-api/. They need to be modified at the same time to mirror the change.

connorskees commented 1 year ago

Is the file in the spec/js-api not auto-generated?

ntkme commented 1 year ago

Is the file in the spec/js-api not auto-generated?

As for as I know they are not auto-generated, therefore the CI has a check to ensure the documentation matches the spec.

connorskees commented 1 year ago

I got CI passing on the sass/sass PR if someone is able to re-run the CI for this PR

connorskees commented 1 year ago

The JS API steps seem to pass now, but the static analysis step is failing with an error that looks unrelated to my changes. Is the failure spurious?

ntkme commented 1 year ago

The JS API steps seem to pass now, but the static analysis step is failing with an error that looks unrelated to my changes. Is the failure spurious?

Because this repository does not have package-lock.json checked in, the issue could be that one of the lint related dependency got a new version and it is breaking the test. For now I guess we will have to debug and find out what went wrong.

In the future, for more stable and reproducible CI runs I think we could check in package-lock.json and use npm ci instead of npm install in tests. The only downside is that dependabot will be noisier about each dependency update, but on the upside this actually give us better test coverage for dependency version changes.

nex3 commented 1 year ago

https://github.com/sass/embedded-host-node/pull/255 should fix the static analysis issues.