Skip to content

Implement sass --embedded in pure JS mode #2413

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

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Oct 26, 2024

Closes #2325.

Implementation

The actual isolate dispatcher and compilation dispatcher are nearly unchanged. However, I had to replace isolate with worker communication, and mock tons of small things that do not work on node.

Testing

  • All Dart embedded tests are passing. - GitHub CI has been updated to run these in this PR.
  • All JS API tests are passing. - GitHub CI has been updated to run these in this PR.
  • All Ruby API tests are passing.

@ntkme ntkme force-pushed the embedded-compiler branch from 8d7d4de to 7084da7 Compare October 26, 2024 02:31
@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
@ntkme ntkme force-pushed the embedded-compiler branch 2 times, most recently from e66df5b to d53fcc5 Compare October 26, 2024 17:27
@ntkme ntkme force-pushed the embedded-compiler branch 9 times, most recently from d7e6206 to b3794eb Compare October 28, 2024 06:35
@ntkme ntkme marked this pull request as ready for review October 28, 2024 06:49
@ntkme ntkme force-pushed the embedded-compiler branch 3 times, most recently from 9112b44 to 54fabf3 Compare October 28, 2024 07:42
@ntkme ntkme force-pushed the embedded-compiler branch 8 times, most recently from 257b4fd to ac718ee Compare October 28, 2024 21:03
@ntkme ntkme force-pushed the embedded-compiler branch from 5231955 to 5aba167 Compare May 21, 2025 20:42
@ntkme
Copy link
Contributor Author

ntkme commented May 31, 2025

@nex3 Any feedbacks on my last two comments above?

@ntkme ntkme force-pushed the embedded-compiler branch from 5aba167 to 3686b63 Compare June 9, 2025 20:56
@ntkme
Copy link
Contributor Author

ntkme commented Jul 16, 2025

@nex3 Pinging again regarding how to deal with legacy js package vs new js_introp:

  • The legacy js package has been officially marked discontinued.
  • Both the legacy js package and new js_introp provides @JS annotations. This issue is addressed in 0.7.0 of legacy js:

    Breaking Change: Moved annotations to single location in dart:_js_annotations. This makes it easier to avoid collisions with dart:js_interop's @JS, as you will now see a static error instead of it resolving in favor of the package definition. However, since this may result in breakages in previously working code, this is treated as a breaking change.

  • Ideally node_interop should be rewritten with new js_interop but nothing is happening. On the other hand, node_interop cannot be easily upgraded to js 0.7.x due to js 0.7.x requires dart >=3.1.x, while node_interop's dev dependencies build_node_compilers do not support dart 3.x at all. This means node_interop is effectively stuck at js 0.6.x as of now, which means we have @JS conflict that we cannot mix old js and new js_interop code in the same file.

So honestly I think the way js interop is handled in this PR is already at best effort. - That new code only uses js_interop and it's currently isolated from the legacy js code.

Any more thoughts? Would you mind take another look at this?

@nex3
Copy link
Contributor

nex3 commented Jul 16, 2025

Sorry that this has taken me so long to finish reviewing, there have been a lot of things to juggle this year.

I'm working on a major change (https://github.com/sass/dart-sass/tree/js-interop) to move Dart Sass to the new JS interop system. This includes creating a new package for defining typings and wrappers for the JS core library (as well as possibly pushing some changes upstream to dart:js_interop) as well as updating node_interop which will probably include dropping its compiler dependency. You can see the work in progress on each of these at https://github.com/sass/dart_js_core and https://github.com/pulyaevskiy/node-interop/tree/js_interop.

At this point it may make sense to wait on landing this PR until that work is finished. We're already closing in on that, so it shouldn't be too much more time.

@ntkme ntkme force-pushed the embedded-compiler branch from 3686b63 to ff957ec Compare July 21, 2025 23:37
@ntkme ntkme marked this pull request as draft July 24, 2025 16:15
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.

Implement sass --embedded in pure JS mode
3 participants