excubo-ag / WebCompiler

Apache License 2.0
149 stars 30 forks source link

Does not cope with underscore imports in a subdirectory #20

Closed mikes-gh closed 3 years ago

mikes-gh commented 3 years ago

In scss compilation the following imports statement

@import 'abstracts/colors'; will compile in abstracts/_colors';

webcompiler does this correctly.

however in webcompilers check to see if it needs to build dependencies it doesn't allow for the underscore.

https://github.com/excubo-ag/WebCompiler/blob/52478c6afe2a21346a9b3370feb21690c101ffcb/WebCompiler/Compile/SassCompiler.cs#L116

so I had to rename all my imports with an underscore. webcompiler should really do its dependency check using the same rules it uses to compile.

stefanloerwald commented 3 years ago

Hi @mikes-gh,

I'm not fully sure whether I understand this issue yet. Is only the dependency detection flawed? I'm already transitioning into the holiday season, so am not trying to understand a complex regex at this point. If you have a fix in mind, please let me know. Ideally it would come with a unit test, so that we can properly track the correctness of the fix.

Thanks & best regards, Stefan

elken commented 3 years ago

https://regexr.com/5j5a7 I'm also not sure I understand.

mikes-gh commented 3 years ago

Ah I see. I assumed wrong with out checking. However I still think there is maybe a bug as it works when I changed my imports. I think maybe when getting the file infos it doesnt strip the underscore? I will look at it but it maybe a while.

elken commented 3 years ago

The project I use this on has underscores in the names too with no issue so I'm not convinced there is one but if you can repro and submit a PR then it would be welcome

mikes-gh commented 3 years ago

https://regexr.com/5j5a7 I'm also not sure I understand.

That regex matches both underscore and non-underscore

But take the senario

@import 'abstracts/colors'; but the filename on disk is abstracts/_colors.scss';

The adding of the underscore is incorrect

This line

https://github.com/excubo-ag/WebCompiler/blob/52478c6afe2a21346a9b3370feb21690c101ffcb/WebCompiler/Compile/SassCompiler.cs#L156

appends an underscore to the string abstracts/colors so it ends up looking for _abstracts/colors.scss instead of abstracts/_colors.scss.

So it appears that the compiler does not adjust for the underscore in the case where the source files are organised into sub-directories

It subsequently fails to find the file and the dependency check fails to see that a compile is required.

mikes-gh commented 3 years ago

Ill see if I can submit a test and fix

elken commented 3 years ago

That's the regex you complained didn't work. As said before I use in a project with both underscores and no underscores with no issue at all.

On 26 Dec 2020, 15:56, at 15:56, Mike Surcouf notifications@github.com wrote:

https://regexr.com/5j5a7 I'm also not sure I understand.

That regex matches both underscore and non-underscore

@import 'abstracts/colors'; but the filename on disk is abstracts/_colors';

The adding of the underscore is incorrect

This line

https://github.com/excubo-ag/WebCompiler/blob/52478c6afe2a21346a9b3370feb21690c101ffcb/WebCompiler/Compile/SassCompiler.cs#L158

appends an underscore to the string abstracts/colors so it ends up looking for _abstracts/colors.scss instead of abstracts/_colors.scss.

So it appears that the compiler does not adjust for the underscore in the case where the source files are organised into sub-directories

It subsequently fails to find the file and the dependency check fails to see that a compile is required.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/excubo-ag/WebCompiler/issues/20#issuecomment-751368899

mikes-gh commented 3 years ago

Hi @elken

Im not complaining 😄 I was just pointing out an issue that may affect others. The issue is not with the regex as I first thought. The problem is specific to relative directories and how webcompiler is adding the underscore to try and find the file. This is confirmed by debugging the code.

mikes-gh commented 3 years ago

So to repro you must have

  1. Imports which contain subdirectory/somescssfile
  2. A file on disk that is subdirectory/_somescssfile.scss

LibSass is fine with this but the check to see if compilation is required does not work in webcompiler. That is why it only seems to compile if the output is deleted.

mikes-gh commented 3 years ago

I had a look at supplying a test but cant get my head round the test inheritance and setup sorry.

elken commented 3 years ago

I'll try and replicate with one in the next few days if I get around to it. Thanks for finding the repro

stefanloerwald commented 3 years ago

I had a look at supplying a test but cant get my head round the test inheritance and setup sorry.

It's perfectly fine to build your test in any way you want. The existing tests mainly deal directly with the build pipeline, so it's centered around that.

mikes-gh commented 3 years ago

It's perfectly fine to build your test in any way you want. The existing tests mainly deal directly with the build pipeline, so it's centered around that.

Test and fix submitted in PR