Improved search

[var-bp]

Hello, I have made little improvement for search. Let say you have this webpack dev config:

const devConfig = {
  // some code
  module: {
    rules: [
      {
        test: /\.(sass|scss)$/,
        use: [
          'style-loader',
          'css-loader',
          'postcss-loader',
      'resolve-url-loader',
      {
            loader: 'sass-loader',
            options: {
              sourceMap: true
            }
          }
        ]
      },
      {
        test: /\.less$/,
        use: [
          'style-loader',
          'css-loader',
          'postcss-loader',
          'resolve-url-loader',
      {
            loader: 'stylus-loader',
            options: {
              sourceMap: true
              use: [require('nib')()],
              import: ['~nib/lib/nib/index.styl']
            }
          }
        ]
      }
    ]
  }
};

Sass files will work just fine, but stylus wouldn't. Because stylus has plugin nib that located in node_modules. The problem in this function: testNotPackage(absoluteStart). When building queue with limit = 'my-project/' we have such results:

/my-project/node_modules/nib/lib/nib
/my-project/node_modules/nib/lib
/my-project/node_modules/nib
/my-project/node_modules/nib/lib/nib/normalize
/my-project/node_modules/nib/lib/nib/text
/my-project/node_modules/nib/lib/nodes
/my-project/node_modules/nib/docs
/my-project/node_modules/nib/iconic
/my-project/node_modules/nib/node_modules

Search stops at _/my-project/nodemodules/nib and dont go up to limit because folder nib has package.json. My proposal to remove testNotPackage(absoluteStart) and fix this very old stylus/nib problem.

[bholloway]

TLDR; I am not opposed to this but we need to be very careful with the file search.

Some search limits are necessary or the search can easily escape and scan your whole HDD. There are edge cases such as npm link'd directories where testNotPackage limit may be the only bound that works. I would prefer to be "hands off" the file search as much as possible.

This project has a convention of backwards-compatibility that needs to be followed if this PR is to be successful. Any change needs to be opt-in. If we find that it is the majority use-case then we deprecate the old way much later on.

Under that convention I would suggest a new option.searchModules:boolean at very least.

However that would still not be usable in some of these edge cases. it would be far better to have something that only goes deeper, and to N-modules depth. For example option.moduleDepth:number. You are welcome to add commits for this that we can discuss.

[bholloway]

Also used here here.

[var-bp]

Thanks for quick replay. Can I commit new feature option.moduleDepth:number in this branch?

[var-bp]

I have committed new feature in this branch, I hope you don't mind.

[bholloway]

Yeah go for additional commits as needed, your prerogative.

I will try to comment whenever I get time. IF it is a work-in-progress just let me know so I can withhold judgement and let you do your thing.

[bholloway]

Having re-read your use-case I think I need some clarification. I don't know enough about nib to see what you are doing.

It seems that the search starts in my-project/node_modules/nib at a stylesheet. That stylesheet references something that can only be found where ?

• in my-project/node_modules/...
• in my-project/src/... (where by src I just mean your project source files or whatever)

[var-bp]

Search starts at my-project/node_modules/nib/lib/nib/index.styl and goes up. Here is content of index.styl, here how to include nib. My project structure:

my-project
  bin // server.js
  config // webpack.config.js
  dist
  node_modules
  src
  package.json

[bholloway]

So @frontEndWebDev I'm thinking this is similar to #27. You have a mixin where the url() content is provided by a variable.

If that is the case I don't think this PR is appropriate. I know that sounds harsh but let me tell you why...

Problem

The majority of the time this loader should be O(1). Given a source-map and a relative URL it should be able to immediately find the file without searching.

There are some edge cases where this does not happen, like used to happen with BootstrapSass, but there the files were in the same package not far from where they should have been.

Using url() in a mixin makes full search the majority case. In your case, you need to escape the package to find the file. Essentially you need to search your whole project O(n), not very performant.

Root cause

At least one other user has reported that the source-map coming out of SASS correctly attributes the text inside the url() statement to where the variable is set. They claim that the problem is with reworkcss.

Solution

The presumed solution is to replace reworkcss with postcss.

Before I do that I think I need to confirm the right info is in the SASS source-map. But I would also like to check the right info is in the Stylus source-map.

Could I propose...

1. You work with your fork in the short-term.
2. You author a small example project using stylus that exhibits your use-case.

Alternative PR ?

If you want to continue with a file-search solution then...

3. Implement a different solution that is typically O(1).

For example, keep the existing file-search limits but allow the user so specify some path or rule where we can do a parallel search. This would be considered a workaround for the mixin problem.

The problem with this is that with a 'feature centric' approach (which I promote) you would not have any single 'assets' directory that you could specify.

[var-bp]

Thank you for your time and help. I will choose 1st solution.

In my humble opinion, if there was no node_modules in the nib folder, the search would still goes from the very bottom to the limit. My modification just removes an unexpected obstacle in the form of node_modules subfolder and does not change the search engine.

This problem is not exclusive for the stylus/nib, it can happen with any preprocessor using a library from the node_modules.

[bholloway]

"Thank you for your time and help. I will choose 1st solution."

Sorry forks suck. And without an example project from you it is less likely that I will have an eventual solution for you.

But if you wish we can discuss further.

"it can happen with any preprocessor using a library"

I completely agree there is a big problem. I think there can be room an imperfect fix that will work for you and others.... and I realise now O(1) is a tough ask.

However I also suspect that searching up N modules depth is not the best solution for what you want to do (since they will be all searched deeply on the way).

Unless I am mistaken, you just need to "enqueue" the root directory unconditionally? From what I can see the search would still not enter packages in node_modules.

i.e. The limit is added to the queue

// start a queue with the path to the root
var queue = pathToRoot.concat(options.allowRoot ? limit : []);

If I had an example project I could try but otherwise I am relying on you.

[var-bp]

Sorry, but I can't publish my project for security reasons. Your workaround solves the problem. At first, I myself thought about such a solution, but found it too simple :) .

Can you publish your change to the main repository? It can help a huge number of people.

[bholloway]

"Can you publish this change to the main repository?"

I think we can do this as a minor version release.

I would suggest whichever of these is easiest for you.

• (force) push that change as the only commit to this branch; or
• create a new PR with that change as the only commit

"found it too simple"

I sort of feel the same.. that there is some logic bomb we are missing with the "simple" solution. Perhaps I will mark it in the readme.md as an "experimental" option for now.

[var-bp]

Whichever you think is most appropriate, I will be satisfied with any option that suggests npm i resolve-url-loader as soon as possible :) .

[bholloway]

go with the first option

[bholloway]

I should clarify that I want you do make the commit so that you get contribution credit.

[var-bp]

Sorry for such delay. I made commit with several improvements:

// start a queue with the path to the root
var queue = pathToRoot.concat(!!options.root && pathToRoot.indexOf(options.root) === -1 ? limit : []);

[var-bp]

Thank you for your help :) !

[bholloway]

Per this blog there is the option to squash when I press the merge button. Presumably it will retain your authorship @frontEndWebDev.

I would prefer you squash your commits yourself now and force-push. But if you are not confident doing that then I can try the squash button.

[var-bp]

Done.

[bholloway]

hmm... I'm still seeing 8 commits. I would normally see just one. I think you will need to do it via rebase (with squash) + forcepush.

[var-bp]

Can you do it? Because for some reason my GitKraken can't.

[bholloway]

Ok @frontEndWebDev github let me force-push to your branch. Please check if you are happy with that.

Although it has my picture on the commit this seems to be in the github UI only. I can confirm that you have the contribution. I think that just happens because I pushed it.

Luckily for me webstorm has a good UI for this sort of thing. What I actually did...

1. Fetch your remote
2. Checkout your branch
3. Reset to the commit before you started working.
4. Cherry pick the commit that you already squashed.
5. Commit (webstorm had auto filled you as author)
6. Force-push to your remote

Normally I would replace 3,4 with rebase (squash) but for some reason I just took your squashed commit.

[var-bp]

It's ok. Sorry for the inconvenience. I'm using a webstorm myself. I'll switch to his Git, because GitKraken sucks.

[bholloway]

Hey no problem. Thanks for your patience with this PR. Lets do this!

[bholloway]

@frontEndWebDev I've published as resolve-url-loader@2.1.0. Please let me know if it explodes or there are any immediate problems.

Thanks for your contribution, pleasure working with you.

[var-bp]

Thank you, it was exciting :) !