node-sass-magic-importer: Transitive imports broken

[ockham]

Quoting @jsnajdr's https://github.com/Automattic/wp-calypso/pull/30933#discussion_r262457316 (which is a succinct gist of my own observations):

[...] So, with the following directory structure:

node_modules/color-studio/index.scss
color-schemes/node_modules/color-studio/index.scss
color-schemes/index.scss

and contents of color-schemes/index.scss:

@import '~color-studio';

the import gets resolved to node_modules/color-studio instead of color-schemes/node_modules/color-studio? That's not how the resolution algorithm should work and it's a bug in node-sass-package-importer.

The node-sass importer function gets the file path from where the import is performed as the second parameter and it should use it to resolve:

function importer( url, prev ) {
  if ( ! url.startsWith( '~' ) ) {
    return null; // not our business to resolve this path
  }
  return resolve.sync( url.slice( 1 ), { baseDir: dirname( prev ) } );
}

[maoberlehner]

Maybe I don't understand the problem correctly but from what I understand about the problem, I tend to disagree.

node_moduls
>> package-a
>>>> _i-import-package-b.scss_ (@import '~package-b.scss')
>> package-b
i-import-package-a.scss (@import '~package-a.scss')

In an example like the above, I always want the packages to be resolved starting from the root directory and I think this is the correct behavior.

This is also practically how npm does it since it moved to a flat dependency tree with v6 (with the exception if it is not possible to use a single version of a certain package).

[maoberlehner]

Thinking a little closer about it, the correct way would be that imports inside of a package do look for their "closest" node_modules directory and use that as the root directory to resolve the dependency. If it is not found in the "closest" node_modules dir, the node_modules dir in the current working directory should be used.

I guess that would fix your problem.

[jsnajdr]

Thinking a little closer about it, the correct way would be that imports inside of a package do look for their "closest" node_modules directory and use that as the root directory to resolve the dependency.

Hi @maoberlehner :wave: Yes, that's how the Node.js resolution algorithm works. Webpack uses the same algorithm to resolve module imports.

import 'package';

first searches for a ./node_modules/package directory relative to the file where the import is. If it doesn't find package there, it goes one directory up. And so on, recursively to the root of the filesystem.

This is also practically how npm does it since it moved to a flat dependency tree with v6 (with the exception if it is not possible to use a single version of a certain package).

If the resolution started at the root directory, it wouldn't be possible for a package to depend on a specific version of another package.

node_modules/react (v15)
node_modules/package/index.js
node_modules/package/node_modules/react (v16)
index.js

If package depends on react@16 and the main app depends on react@15, this is how NPM will lay out the directory tree.

import 'react' in node_modules/package/index.js will load v16, import 'react' in root index.js will load v15.

[mpdude]

I have run into this, and reading the comments I think we all agree it should work like described in https://github.com/maoberlehner/node-sass-magic-importer/issues/188#issuecomment-470979774?

[mpdude]

@maoberlehner are you still maintaining this package and would you be able to review and/or accept a PR?