[Feature Request/Discussion] Add Option To Disable Exhaustive Search of Source Dirs if Path Can't Be Resolved Relatively

[mvastola]

Per the README.md:

Usually the asset is found relative to the original source file O(1). However in some cases there is no immediate match (cough bootstrap cough) and we so we start searching both deeper and shallower from the starting directory O(n).

exhaustive searching of source directories is done as a workaround to accommodate bootstrap (and other modules?) which do not specify asset paths correctly.

This workaround, however, has a few shortcomings, one of which is it adds to compute/compile time. Additionally the README.md for sass-loader states there is an alternate solution to the bootstrap issue:

Library authors usually provide a variable to modify the asset path. bootstrap-sass for example has an $icon-font-path. Check out this working bootstrap example.

A more problematic shortcoming was just identified for those users who use this module in conjunction with rails apps via the webpacker gem and yarn. See rails/webpacker#932.

In light of this, I was hoping to begin a discussion/inquiry into whether a PR would be accepted that added the capability to disable exhaustive searching, or if there were some other means of assuaging the concerns raised in rails/webpacker#932.

Thanks a ton! Mike Vastola

[bholloway]

@mvastola That sounds like a good idea. I am certainly open to features that are opt-in.

I have a feature branch where I have refactored in order to support postcss and other engines in future releases. I would really like to do anything new against that as it is pretty much ready to go.

Plus if you test it then that means I owe you.... which brings us to your feature :wink:

---

It sounds like you want some strict or immediate (or name?) flag that inhibits searching, or some attempts integer that limits the amount of searching.

You will see that the value-processor.js calls the find-file.js which works through a queue of candidates.

We need to tread carefully in this file. Any change to the search routine has historically lead to regressions. However now that we are discussing it, i can see there is probably some stuff I can clean up around the edges.

At first glance my thinking...

cleanup (for me)

• Move the third limit argument up into the lexical scope here, since it is common.
• Consider combing the base and absolute functions, since only one is actually used.

feature (for you if you want credit, else for me)

• Add the new option at the top level.
• Probably just break the iteration here, or make some other light-touch.

Let me know your thoughts on this.

[mvastola]

Some answers to your questions:

• Yes, it's perfectly fine that the feature is optional/opt-in. Given how we append your loader to the webpack environment, we can easily pass in config settings.
• I'm absolutely happy to test it once done.
• I'm not too picky on the name. There shouldn't be any 'searching' at all per-se (depending on how you define searching, I guess). There are one or more paths to be checked (the number is equal to the size of the stack of @imports from the entry file to the file containing the url() directive). See this comment for an example.
• It's probably better if you do this. I think it involves a little more tweaking than I'm comfortable with. Specifically because it involves an iteration through the source map (and I know nothing about source map parsing) rather than simply an iteration through the directory hierarchy. Again, I'm more than happy to confirm it's working once all is said and done, however.

[bholloway]

"There shouldn't be any 'searching' at all per-se"

Yes good point... So we just assume the file is where we expect and not even validate its presence.

Casting my mind back, the search was simply to work around bootstrap (at the time). Hence the readme.md comment.

Maybe we just go naive mode and rewrite. If Webpack cannot find the file it will tell us either way.

My only concern is where there is a url() that is not a file. We have always ignored data: prefix, and recently we have started ignoring https?: prefix. But is that enough? The value might actually be a relative or root-relative URL in the final deployment.... Hmmm I will think on it.

[mvastola]

Well by no 'searching' I more meant there would be no Indiscriminate checking of all directories in a hierarchy. As I detailed, there will be a few explicit paths to check, and if the path is located relative to a file in the stack, the path should be replaced so that the url points to the image/font asset are relative to the css entry file. Or am I missing something?

I wouldn't call this naive at all. If anything, it's the right way to go about it because url paths are supposed to be relative to the css file inherently. The bootstrap thing was probably a necessary workaround, but you might even find that this change makes for a good default. (That is beyond what I'm asking for, however.)

[mvastola]

So doing a bit more research, and thinking about this more, I want to amend what I said slightly. I don't think it makes sense (or is intuitive, or easily doable) to check for assets relative to the path of every stylesheet on the stack of @imports. I think only the entry file and the top of the stack should be checked (with the latter given priority).

It looks like right here you determine the directory of the entry file, and path.dirname(declaration.position.source) would (from what I gather) give you the directory of the file with the url() directive. So those would be the two candidates. Then it's just a matter of testing if the latter exists and, if not, deferring to the former, if I'm not mistaken.

[bholloway]

@mvastola I am playing with an attempts option, that you would set to 1. This will still validate the file is present and do nothing if it is not found. However it won't search further.

Whether the loader should just naively rewrite the URL (without testing) is a secondary. Hopefully we can avoid that for your use case. I will let you know when there is something you can try.

I have narrowly caught some refactor errors on the rewrite branch, which I am basing this work off. Therefore, even if you test this code, it will probably be in the next major release and be 2.0.0-rc prerelease version, until I can get some tests written. How would that sit with you?

[mvastola]

Almost forgot about this comment.

I have narrowly caught some refactor errors on the rewrite branch, which I am basing this work off. Therefore, even if you test this code, it will probably be in the next major release and be 2.0.0-rc prerelease version, until I can get some tests written. How would that sit with you?

I mean we certainly appreciate the work you're giving this, and if you're refactoring/rewriting your code, I understand you not wanting to have to backport this fix (I don't know how easy this would be in any case, and presumably this is unknowable until the fix against the rewrite branch is finished.)

I have no authority to make the call as to when exactly webpacker will start to use a different version constraint with regard to resolve-url-loader. I imagine (though I may stand corrected), as with any upgrade, it will depend on a number of things:

• How stable the new release is
• If there are standing issues (such as this one) that are only resolved in the newest release
• Whether or not any breaking changes are involved
• The release schedule of webpacker (if breaking changes are involved, they might wait until a minor/major version jump)
• The devs' desire for "adventure" :-p
• Etc

In any case, this is really a question for @jeremy and @gauravtiwari and above my pay grade (to the extent that that phrase has any meaning in conjunction with FOSS).

[bholloway]

Published as 2.2.0

[bholloway]

@mvastola Let me know any problems and I will reopen.

I leave it to you to continue being the interface with rails webpacker, feel free to tag me in that discussion. I will consider what we are doing with 3.0.0 and tag you on those PRs if I think it is interesting.

[mvastola]

Thanks so much! Making the PR for webpacker now.

[mvastola]

@bholloway PR is rails/webpacker#980, FYI.

[mvastola]

.. and it's merged!