Implement sass --embedded in pure JS mode - #2413
Conversation
8d7d4de to
7084da7
Compare
e66df5b to
d53fcc5
Compare
d7e6206 to
b3794eb
Compare
9112b44 to
54fabf3
Compare
257b4fd to
ac718ee
Compare
d0c8c1f to
5fa8f0d
Compare
5fa8f0d to
7c9069e
Compare
|
@nex3 I can see that the team is focusing most of the time on the postcss sass parser and this pull request has low priority. However, I would like to get this done in next few month before dart 3.8 become stable so that we can offer this as a replacement for dart ia32. Please take a look when you get a chance and let me know if you have any questions. |
|
Would really be great to see this merged to have embedded support on FreeBSD even if in pure JS mode. Thank you! |
There was a problem hiding this comment.
Finally managed to carve out some time for this. I haven't done a complete review yet, but this should be enough to get you started.
There was a problem hiding this comment.
Another partial review, but I think we're making good progress!
There was a problem hiding this comment.
This can also just go in the existing io catch-all.
There was a problem hiding this comment.
This should be part of the existing IO library (which already has definitions for exitCode).
There was a problem hiding this comment.
Well, I was trying to implement this with new js interop from dart 3.3 without relying on the node_interop package which heavily depends on deprecated dart:js_util, which will start to break at some point.
For exitCode I can use the existing one we have.
What about Stdin, Stdout, Stderr? These are effectively wrapper classes on node stdio to create an interface that's same as the dart built-in classes while it only partially implement necessarily methods we need on those interface. @nex3 What's your preference here? Create another interface that wraps the original dart interface?
There was a problem hiding this comment.
Any work to expose existing JS APIs should go in lib/src/js.
There was a problem hiding this comment.
Again, this is a saparate file because it's new js interop from dart 3.3, which I found a bit awkward to put it together with the classic interop code in the same place. Any recommendation on how to deal with it?
|
Applied most of the feedbacks. However, I have a awkward feeling about mixing classic js interop and new js interop in the same file, which is why I have lots of js specific things with new interop are only in embedded folder instead of in the generic folder. @nex3 I haven't changed these yet, as I want to hear what's your thought on this. |
|
Another note: The reason I tried to avoid classic interop and node_introp is that I couldn't get The new dart 3.3 js interop on the other hands was way more straight forward to use and debug. I got |
|
@nex3 Any feedbacks on my last two comments above? |
|
@nex3 Pinging again regarding how to deal with legacy js package vs new js_introp:
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? |
|
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 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. |
|
Rooting for this, as I can't use embedded sass on FreeBSD. |
Me too as I use it on FreeBSD and it works great. |
|
The JS interop work is close to being done, at which point this will be unblocked. |
|
Would just like to add on top of using this to get Discourse on FreeBSD I just got Vikunja working. Both work flawlessly thank you for this change it's been a huge help. |
|
@nex3 Any updates on the |
|
The only real blocker is merging a bunch of my JS type definitions which are currently living in a private package into Dart team owned sources. I'm in discussion with @srujzs about the best way to do that, and I may pitch in to help land it. If you'd like to help as well, we could probably loop you in. |
|
Hi @nex. Do you have an ETA for this PR? |
|
Is this still planned to be merged? I use this frequently on FreeBSD and it's always worked perfectly it would be fantastic to see it merged as part of the base. Thank you as well @ntkme for maintaining this as well as you do it's greatly appreciated. |
|
@verm Yes, but it's blocked on getting Dart Sass working with the new Dart JS interop system, which is itself a long process. |
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
Current Status
Functionally it's ready to merge. Cosmetically it's waiting on dart-sass (depends on node_interop) to migrate off the legacy js interop to avoid the confusion of mixing legacy and new js interop, which is currently blocked by dart-lang/sdk#63403
Because FreeBSD support has been requested a lot for ruby's sass-embedded gem, this patch has been bundled within sass-embedded gem.