limit file search by `attempts` option (back-port to v2)

[bholloway]

Back-port the attempts option to v2. See #69 and #70.

Changes should be minimal to guarantee (by inspection) non-breaking enhancement.

[bholloway]

@mvastola Got bored tonight so did the back-port, wasn't too bad.

Eyeballing these changes it seems pretty safe.

I have basically tested it and the attempts seems to be operating. However you please also give it a sanity check by testing with your project.

The more I think about it, the more I like the idea of...

1. Release minor 2.2.0 with these changes, meaning the attempts=0 back-port.
2. Release major 3.0.0 as the refactor branch with attempts=1 default value.

Essentially "remove the magic" gives me an excuse for a breaking change, in which I can hide a refactor.

Given your recent testing (in addition to my own) I have reasonable confidence all will go well. But worst case there would be a solid 2.2.0 to use until the 3.0.x got fixed.

[mvastola]

Thought: you might want to add a deprecation warning on the minor version upgrade. For instance, if the user has not set attempts at all and any of the files found is found beyond the first attempt, maybe you want to trigger a warning? I realize this potentially requires slightly more logic (since you need to determine whether or not the attempts option was explicitly passed) and it doesn't matter to webpacker, but it just occurred to me as something that might be a good idea for your users in terms of forward-compatibility. Entirely up to you though.

[bholloway]

That is a very good thought @mvastola .

Following our discussions I am thinking 95% of users should just get off the magic, period. In that case I would not want them to specify the extra search options like attempts, root, includeRoot, etc.

My plan was to release 2.2.0 with the advice "set attempts=1". And immediately release 3.0.0 with advice "breaking change: use attempts if you want the magic back".

Really the only reason for 2.2.0 is to give people a semver choice, and as a backup in case 3.0.0 goes horribly wrong.

That leads me to think I should not nag users on an option that should largely be irrelevant/omitted going forward. But I am definitely going to think more on this, because like I said it is a good thought.

[mvastola]

Oh, I agree that they shouldn't be setting attempts, but the deprecation warning can also be phrased to -- instead of nagging them to change their config, nag them to fix their code, which is the proper way to go about it.

[bholloway]

Ah sorry, I reread and I now understand. That's a really awesome idea. I hope I can make it work.

[mvastola]

Well for the first part, distinguishing between an unset attempts and a 0 attempts, the best way would probably be to set the default to null, and changing this line to:

var remaining = options.attempts == 0 ? 1E+9 : options.attempts;

Then just add an remaining && in front of the remaining-- here.

Then it's just a matter of checking if it's past the first attempt when it succeeds.

[mvastola]

Actually, I have an idea for a much cleaner way to patch this. I'll make a PR in a few.

[bholloway]

My concern is that failure to find file just leaves the url() unchanged. Which does not necessarily fail the build. I think we only want to nag if the build fails.

But maybe we could do something here, its hard to know. Historically these warnings are to do with failure to parse CSS, not failure to find assets.

[mvastola]

I disagree. If the build fails, there's no need to nag. The build failing is enough of an error and the fact that the build failed is not an indication that the app is going to have a forward-compatibility problem.

The reason to nag is if the file is found, but only after more than one attempt. That means a deep search has begun, which won't happen by default on a future build.

I'm almost done with the PR, so I'll show you what I'm thinking.

[mvastola]

See #72

[bholloway]

"which won't happen by default on a future build."

Yes. However I was thinking that simply having a breaking change in semver causes people to look a fresh at their build. If you remove magic and people have a problem they can just put magic back in.. but they are aware of it.

I think there is an expectation of this sort of "fix" when you make a major release. I am happy to inconvenience users to that degree.

[mvastola]

I mean users will be inconvenienced either way (and I agree it's fine). I think this just warns them exactly where the problem is and tells them how to fix it before they take the plunge into the next release. That's the whole point of a deprecation warning -- to make the eventual upgrade smoother by giving users advanced notice to remove/modify code/functionality that is being phased out.

[mvastola]

See my most recent comment in #72. I think you might be misunderstanding the message slightly. The intent isn't for them to turn attempts back on. It's for them to fix the path in their code.

[bholloway]

@mvastola Sorry for the delay.

I think you tested this at some point. However if you are unsure please give it a whirl before I merge it tomorrow.

[bholloway]

Published as 2.2.0