Skip to content

Support pure JS sass executable#344

Merged
nex3 merged 5 commits intosass:mainfrom
ntkme:embedded-compiler
Jul 21, 2025
Merged

Support pure JS sass executable#344
nex3 merged 5 commits intosass:mainfrom
ntkme:embedded-compiler

Conversation

@ntkme
Copy link
Contributor

@ntkme ntkme commented Oct 26, 2024 •
edited
Loading

sass/dart-sass#2413

This PR changes how compiler is setup for local development. Instead of symlink the compiler into a special vendor directory:

  • Dart: link build/dart-sass/build -> node_modules/sass-embedded-${platform}-${arch}/dart-sass.
  • Pure JS: link build/dart-sass/build/npm -> node_modules/sass.

Only one kind of compiler will be installed by init.ts, that the other kind will be removed if exists.

In the compiler-path.ts, loading the compiler is attempted in the order of:

  • dart runtime + snapshot for release (bypassing the script for slightly better performance)
  • dart sass wrapper script for local development
  • node + sass.js for platforms without dart support

@ntkme ntkme mentioned this pull request Oct 26, 2024
Draft
@ntkme ntkme marked this pull request as ready for review October 26, 2024 02:50
@ntkme ntkme marked this pull request as draft October 26, 2024 03:40
@Thefrank Thefrank mentioned this pull request Oct 26, 2024
Closed
4 tasks
@ntkme ntkme closed this Oct 28, 2024
@ntkme ntkme deleted the embedded-compiler branch October 28, 2024 00:27
@ntkme ntkme restored the embedded-compiler branch October 28, 2024 03:43
@ntkme ntkme reopened this Oct 28, 2024
@ntkme ntkme marked this pull request as ready for review October 28, 2024 03:46
ntkme
ntkme commented Oct 28, 2024
View reviewed changes
@ntkme ntkme mentioned this pull request Nov 4, 2024
Merged
@Goodwine Goodwine requested a review from nex3 December 10, 2024 21:05
@ntkme ntkme changed the title Implement sass --embedded in pure JS mode Support pure JS sass executable Jan 14, 2025
ntkme
ntkme commented Apr 9, 2025
View reviewed changes
lib/src/compiler-module.ts
Copy link
Contributor Author

@ntkme ntkme Apr 9, 2025

Choose a reason for hiding this comment

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

The reason to split this into a different module is that compiler-path contains IIFE that may throw on load, but in tool scripts, we need to know the compiler-module name to setup dev environment without throwing. Therefore it's moved to a separate file.

@ntkme ntkme mentioned this pull request May 20, 2025
Merged
@ntkme
Copy link
Contributor Author

ntkme commented Jul 16, 2025

Because this PR assumes that the dartvm and dart2js sass-compiler will behave exactly same from an external viewpoint, this PR only run test the against dartvm in the CI. The dart2js based CI test is configured in the dart-sass repo itself.

Therefore, this PR has no dependency on dart-sass side of PR, and can be merged independently. - I would like to have this PR merged first, so that I don't have to keep resolving merge conflict every time the version number is bumped.

Of course, if we merge this PR first before merging the dart-sass PR, the API mode won't work. However, at least it allows users to run npm exe sass-embedded consistently on all platforms without native dart support.

@nex3 With the above said, it would be nice if you can take a look and merge this, even if we don't merge dart-sass PR right away.

nex3
nex3 approved these changes Jul 16, 2025
View reviewed changes
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

I'm on board to land this first.

However, at least it allows users to run npm exe sass-embedded consistently on all platforms without native dart support.

I don't completely understand this. It looks like this PR only affects local development. How will it affect what users can do at all?

@ntkme
Copy link
Contributor Author

ntkme commented Jul 16, 2025

However, at least it allows users to run npm exe sass-embedded consistently on all platforms without native dart support.

I don't completely understand this. It looks like this PR only affects local development. How will it affect what users can do at all?

Today if you run npm exe sass-embedded it starts the wrapper script which effectively runs the native dart-sass command line, providing the same end-user experience as npm exe sass. However, before this PR on unsupported native platform (e.g. FreeBSD), this will simply fail due to the wrapper won't be able to find a native sass binary. After this PR, the wrapper will fallback to the dart2js version of sass binary as last resort, so that the command will work consistently.

@ntkme
Copy link
Contributor Author

ntkme commented Jul 16, 2025

Code review feedbacks have been applied.

@nex3
Copy link
Contributor

nex3 commented Jul 18, 2025

Today if you run npm exe sass-embedded it starts the wrapper script which effectively runs the native dart-sass command line, providing the same end-user experience as npm exe sass. However, before this PR on unsupported native platform (e.g. FreeBSD), this will simply fail due to the wrapper won't be able to find a native sass binary. After this PR, the wrapper will fallback to the dart2js version of sass binary as last resort, so that the command will work consistently.

Wouldn't that require us to start shipping a "binary" package that adds a dependency on sass?

@ntkme
Copy link
Contributor Author

ntkme commented Jul 19, 2025 •
edited
Loading

Wouldn't that require us to start shipping a "binary" package that adds a dependency on sass?

I added sass directly as an optional dependency. We can create a “binary” package that simply depends on “sass”, but due to how npm select optional dependencies, technically we need two separate packages to cover all systems:

  • { "os": ["!android", "!darwin", "!linux", "!win32"] }
  • { "cpu": ["!arm", "!arm64", "!riscv64", "!x64"] }

Let me know if that if preferred over having “sass” itself as direct optional dependency.

@nex3
Copy link
Contributor

nex3 commented Jul 19, 2025

Since sass is fairly heavyweight in terms of filesize, I think I'd rather have separate "binary" packages.

@ntkme ntkme requested a review from nex3 July 19, 2025 01:33
@ntkme ntkme mentioned this pull request Jul 19, 2025
Merged
nex3
nex3 approved these changes Jul 19, 2025
View reviewed changes
npm/all-unknown/README.md
Copy link
Contributor

@nex3 nex3 Jul 19, 2025

Choose a reason for hiding this comment

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

Since these packages behave unusually relative to the binary packages, they should probably have slightly more descriptive READMEs and npm descriptions.

Copy link
Contributor Author

@ntkme ntkme Jul 19, 2025

Choose a reason for hiding this comment

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

Updated. Feel free to further amend the language if you have any other preference.

@ntkme
Copy link
Contributor Author

ntkme commented Jul 19, 2025

Note, here is the PR to update the dart-sass side of CI pipeline for this: sass/dart-sass#2609

@nex3 nex3 merged commit 4c312c4 into sass:main Jul 21, 2025
17 checks passed
@ntkme ntkme deleted the embedded-compiler branch July 21, 2025 23:35
@jattasNI jattasNI mentioned this pull request Aug 5, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nex3 nex3 nex3 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ntkme @nex3