Bug in path resolution introduced in v9.0.x (import loop detected where there is no loop)

[birdofpreyru]

• Operating System: Ubuntu 18.04
• Node Version: v12.18.2
• NPM Version: v6.14.5
• webpack Version: v4.43.0
• sass-loader Version: v9.0.1

Actual Behavior

On an existing codebase, after upgrade from [email protected] to v9.0.1 I am getting the error:

SassError: An @import loop has been found:
               src/styles/global.scss imports src/styles/mixins.scss
               src/styles/mixins.scss imports node_modules/@dr.pogodin/react-utils/mixins.scss
               node_modules/@dr.pogodin/react-utils/mixins.scss imports src/styles/mixins.scss

There is not loop though, just SCSS files with the same names on the same routes in a library, and in the host codebase, i.e. such file structure, which worked fine with sass-loader <= 8.0.2:

/src
  /styles
    - global.scss
    - mixins.scss
/node_modules
  /@dr.pogodin
    /react-utils
      - mixins.scss
      /src
        /styles
          - mixins.scss  

I guess, I'll come with extra details later. Posting the issue in case from the issue description you may know what changes in the latest versions could cause it.

[birdofpreyru]

Narrowed the issue down to the commit aeb86f0: the issue appears with that commit, and everything works fine with the previous commit. Considering the commit is named fix: resolution logic (#839), I guess that fix actually breaks stuff, and also my issue is a legit bug in sass-loader.

[alexander-akait]

Maybe you can create reproducible test repo? it is hack to solve architectural problems with importer and includePaths. It is very strange, can you try to compile it without sass-loader? What is process.cwd()? What is options for the loader?

[birdofpreyru]

@evilebottnawi Sure just try to build this: https://github.com./birdofpreyru/react-starter with the latest sass-loader.

If you just clone, npm install && npm run dev - it works good.
If you then npm install sass-loader@latest && npm run dev - it fails

[birdofpreyru]

Regarding the options, I don't do anything special. Here is sass-loader in my Wepback config template: https://github.com./birdofpreyru/react-utils/blob/master/config/webpack/app-base.js#L239-L244

[alexander-akait]

Here interesting case, by default sass tries to resolve process.cwd() + 'src/styles/mixins' - https://github.com./sass/dart-sass/blob/master/lib/src/importer/node/implementation.dart#L152, we emulate this behavior, here is the problem:

1. We have process.cwd() + 'src/styles/mixins'
2. We have @import "src/styles/mixins"; in @dr.pogodin/react-utils/mixins.scss

I will investigate this problem more deeply

[birdofpreyru]

Hmm... but, why does it do process.cwd() + 'src/styles/mixins'? According to SASS docs, if I got them correctly:

Imports will always be resolved relative to the current file first, though. Load paths will only be used if no relative file exists that matches the import. This ensures that you can’t accidentally mess up your relative imports when you add a new library.

https://sass-lang.com/documentation/at-rules/import#load-paths

[alexander-akait]

@birdofpreyru Don't know, because https://github.com./sass/dart-sass/blob/master/lib/src/importer/node/implementation.dart#L152, p.absolute(path) return absolute path - https://github.com./dart-lang/path/blob/master/lib/path.dart#L119, maybe I wrong, doing debugging

[alexander-akait]

Bug, we don't need to emulate process.cwd() + 'src/styles/mixins', when url contains empty or file schema https://github.com./sass/dart-sass/blob/master/lib/src/importer/node/implementation.dart#L98

[alexander-akait]

@birdofpreyru Looks like it is bug on node-sass, because migration on sass (dart) solve this problem, we strongly recommend do it here. Not sure it can be fixed, because it is the limitation, node-sass has many of them

[alexander-akait]

Found changes:

1. node-sass call our importer before relative resolving, context resolving, includePaths resolving and SASS_PATH env variable resolving - https://github.com./sass/libsass/blob/16f76e2cd6cebf0a31f579a40e635c309109e4db/src/parser.cpp#L357
2. sass (dart) call our importer after relative resolving - https://github.com./sass/dart-sass/blob/master/lib/src/importer/node/implementation.dart#L72

No information about these changes in docs

/cc @nex3

[alexander-akait]

Found workaround, patch release will be today

[birdofpreyru]

Awesome! Many thanks @evilebottnawi !
I actually, probably, should indeed migrate to dart-sass anyway.

[nex3]

Found changes:

1. node-sass call our importer before relative resolving, context resolving, includePaths resolving and SASS_PATH env variable resolving - https://github.com./sass/libsass/blob/16f76e2cd6cebf0a31f579a40e635c309109e4db/src/parser.cpp#L357
2. sass (dart) call our importer after relative resolving - https://github.com./sass/dart-sass/blob/master/lib/src/importer/node/implementation.dart#L72

No information about these changes in docs

/cc @nex3

The JS API importer docs describe this behavior, including the node-sass incompatibility if you click the drop-down arrow on "Compatibility (Import order)".

[alexander-akait]

@nex3 thanks, I never knew that this arrow opens 😄