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

add first class mixins #253

Merged
merged 6 commits into from
Oct 5, 2023
Merged

Conversation

connorskees
Copy link
Contributor

@connorskees connorskees commented Sep 29, 2023

Tracking issue: sass/sass#626
Protobuf changes: sass/sass#3674
Spec tests: sass/sass-spec#1933
dart-sass changes: sass/dart-sass#2073

Closes #252

Comment on lines +77 to +79
`npm install` and then `npm run init`. This provides the following options for
`compiler` (for the embedded compiler), `protocol` (for the embedded protocol),
and `api` (for the JS API):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content change here is to add "npm install and then ". The rest of the diff is reflowing to 80 characters.

@connorskees
Copy link
Contributor Author

cc @nex3

lib/src/value/mixin.ts Outdated Show resolved Hide resolved
lib/src/value/mixin.ts Outdated Show resolved Hide resolved
@connorskees
Copy link
Contributor Author

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

@connorskees
Copy link
Contributor Author

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
Copy link
Contributor

ntkme commented Oct 3, 2023

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

@connorskees
Copy link
Contributor Author

I made the change in sass/sass#3674, but CI fails there I believe because the spec file is in the accepted/ folder and not spec/.

@ntkme
Copy link
Contributor

ntkme commented Oct 3, 2023

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
Copy link
Contributor Author

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

@ntkme
Copy link
Contributor

ntkme commented Oct 3, 2023

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
Copy link
Contributor Author

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

@connorskees
Copy link
Contributor Author

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
Copy link
Contributor

ntkme commented Oct 3, 2023

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
Copy link
Contributor

nex3 commented Oct 4, 2023

#255 should fix the static analysis issues.

@nex3 nex3 merged commit 9e354f0 into sass:main Oct 5, 2023
16 checks passed
@nex3 nex3 mentioned this pull request Oct 6, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for first-class mixins
3 participants