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

Added sass-embedded and swapped to async compiler api #162

Closed
wants to merge 1 commit into from

Conversation

NathanBeddoeWebDev
Copy link

@NathanBeddoeWebDev NathanBeddoeWebDev commented Jan 29, 2024

See discussion @ #160

Also see sass/sass#3296 for background into this issue, and links to proposals and PR's.

@glromeo
Copy link
Owner

glromeo commented Jan 30, 2024

Thank you Nathan
I ran my benchmark (the one in fixtures) and with my rig

my old branch does
built in: 2.450s ..on the first build
then
built in: 176.849ms ...on rebuilding due to a TS change
built in: 173.599ms ...on rebuilding due to a SCSS change

when running it in your branch it does
built in: 734.365ms
then
built in: 236.434ms
built in: 131.038ms
built in: 179.46ms
built in: 177.125ms
...on rebuilding due to a TS change
built in: 1.080s
built in: 1.396s
built in: 998.105ms
...on rebuilding due to a SCSS change

Now... I am pleasantly surpised by the initial build speed improvement 😮 and puzzled 🤔 by the
subsequent being slower when actually caching and having the compiler already initialized should
deliver opposite results.

For TS changes it's pretty much the same and that makes sense but sass...hmm

I am eager to merge your PR let me get my head around this first, ideally I want to merge once and have
a proper improvement across the board... If I can't find out the issue I will merge anyway because I reckon
the overall experience will be much better!

Good job man!!! 👍

@glromeo
Copy link
Owner

glromeo commented Jan 30, 2024

there must have been something going on earlier on because now I get more consistent results
for both TS and SCSS changes

built in: 816.274ms

built in: 181.181ms
built in: 156.739ms
built in: 175.843ms
built in: 151.272ms
built in: 153.187ms
built in: 152.261ms
built in: 156.768ms
built in: 145.372ms

@NathanBeddoeWebDev
Copy link
Author

Wow, that's quite the increase there on subsequent builds...
I've got a couple examples on my large project while watching. Note: I double checked that the dart.exe wasn't starting on each subsequent save, and it appears to be maintaining the same process.

Old Sass Plugin:
image

Embedded Sass Plugin
image

@glromeo
Copy link
Owner

glromeo commented Jan 30, 2024

Yup... I am impressed man!
Now there are just the tests in the way to merge I am having a look from my end to see if I can help

@NathanBeddoeWebDev
Copy link
Author

Is coverage failing? Not sure why that'd be, but I'm seeing test results, and then it's hanging instead of showing me test coverage results.

@glromeo
Copy link
Owner

glromeo commented Jan 30, 2024

😆 this is not the branch I was benchmarking with
the main branch is good... this branch is very slow (10x)

@glromeo
Copy link
Owner

glromeo commented Jan 30, 2024

good news tests are failing (here) mostly because of the embedded using \r in windows...

@glromeo
Copy link
Owner

glromeo commented Jan 30, 2024

can you please create another PR from main?

@glromeo glromeo closed this Jan 30, 2024
@NathanBeddoeWebDev NathanBeddoeWebDev deleted the fork branch January 30, 2024 01:30
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.

2 participants