Conversation
|
I also added directory support. But without a graph for caching, out speed is still quite fast. Note that our version is a bit unfair as we do not generate source map files like dart-sass does. |
|
Those timings are quite striking. For me, |
|
No, very likely because we process 4 input files (entry points) without caching, dart-sass have cache but we need to process each dependency again, so if each file use the same logs dbg in main and import |
|
I am not quite sure how the original |
|
@connorskees What's next on this? |
| } | ||
|
|
||
| /// Check if string ends with sass or scss. | ||
| fn is_xcssfile(s: &str) -> bool { |
There was a problem hiding this comment.
I'm not sure I understand why this function is named the way it is
There was a problem hiding this comment.
Because it checks for both sass and scss so it means xcss, x means unknown. Maybe you have something better in mind?
There was a problem hiding this comment.
Could we do is_scss_or_sass? It's only a few characters longer but it should make its purpose much clearer.
The implementation could likely be simplified to s.ends_with(".sass") || s.ends_with(".scss"). Is there a reason to prefer the iterator over that? I would assume the simpler version would be optimized better, though I could be wrong
|
Ping @connorskees I would say even after this patch it is not good yet, the way the original dart-sass does take it a variety of input format, I don't quite like the way. But it could be improved later on, this just allows easy directory update but good enough to do the basic stuff. PS Oh I didn't know there is conflict, maybe you want to review it first to see if I should fix the conflict or if there any other changes you would like? |
|
My apologies for letting this go for so long. I am reticent to make this change only because I am wary of adding an additional dependency, and am a bit surprised that we are so far behind If there is only 1 compilation, why does caching play into this? I am still very confused as to the role caching plays in all of this. What are we caching? Why? Why is |
| } | ||
|
|
||
| /// Check if string ends with sass or scss. | ||
| fn is_xcssfile(s: &str) -> bool { |
There was a problem hiding this comment.
Could we do is_scss_or_sass? It's only a few characters longer but it should make its purpose much clearer.
The implementation could likely be simplified to s.ends_with(".sass") || s.ends_with(".scss"). Is there a reason to prefer the iterator over that? I would assume the simpler version would be optimized better, though I could be wrong
| criterion = { version = "0.3.3", optional = true } | ||
| indexmap = "1.5.0" | ||
| lasso = "0.3.1" | ||
| walkdir = { version = "2.3.1", optional = true } |
There was a problem hiding this comment.
Is it possible to use std::fs::read_dir instead? Is there a functionality / performance improvement that we really need from this library?
There was a problem hiding this comment.
Yes, we need to recurse a directory, which read_dir does not provide. dart-sass uses a recursive read_dir too. We do this for feature parity.
| let output = match matches.value_of("OUTPUT") { | ||
| Some(output) if is_xcssfile(output) => { | ||
| eprintln!("output must be directory if input is directory"); | ||
| std::process::exit(1) |
There was a problem hiding this comment.
Do you know of a better way to exit early here with an error? This is admittedly a failing in the original code, and ideally at some point we wouldn't use std::process::exit.
There was a problem hiding this comment.
We could use panic but the output might not be 1 (127 IIRC), dart errors with 64 but I don't think that is a good idea. I personally think error code should be well defined or just use 1 which is the normal error.
There are 4 compilations like I mentioned above. And a lot of the dependencies may be reused. I could add that later.
Like I mentioned above since we processed 4 input files. #27 (comment) |
We could do both method, they should be the same. I did this because last time there was 3 items, "sass", "scss" and "css" which makes it long. Do I still need to do this change? Later we may want to add "css" back since that is what dart-scss do. |
2 participants
No description provided.