search

bholloway / resolve-url-loader

Webpack loader that resolves relative paths in url() statements based on the original source file
563 stars 71 forks source link

Added deprecation message for planned `attempts` default in v3 #72

Closed mvastola closed 6 years ago

mvastola commented 6 years ago

So I haven't tested this yet, and feel free to tweak, but this is what I'm proposing.

bholloway commented 6 years ago

I see what you are doing. However I want to just pause for a moment and check if we are both on the same page @mvastola. So forgive me if I digress and take a high level view....

Personally I think this relates more to the 3.0.0 release rather than the 2.2.0 back-port.

In 2.2.0 the search would be in place by default. It is not that this isn't useful. However I think this nag feature is primarily to let 3.0.0 users know that there is "magic" they can add.

Meaning that I think we should look to where this sits in the feature/multiple-engines file structure... This may or may not change your thinking.

mvastola commented 6 years ago

I agree it relates more to the 3.0.0 release but its not to let them know about the "magic" they can add (as we discussed, we want to discourage the "magic" -- ideally IMHO it should be eventually removed altogether). My thinking is that we should warn people currently using the magic in the 2.x.x series that the magic is about to disappear if they don't have attempts set. Maybe this isn't clear and the text of the warning needs to be tweaked?

If you do decide to completely purge the "magic" from your codebase (in, say, 4.x) then this PR should be of course forward-ported into the 3.x.x series with a slightly different warning.

Do you see what I'm saying?

bholloway commented 6 years ago

Hmmm I think so. However I think it might just be easier to just nag to set attempts explicitly.

I have been in a situation of developing build tools for multiple teams. You don't want some configuration aspect to only surface when one of those teams is already using the tool (and makes a bad import). Better that you tell up-front.

That said, I am now not sure I want to nag on a minor release.

My thinking is actually heading to the point that for 3.0.0 the "magic" is actually in a separate node package. So that there is actually some stats on its use. The major release is expected to be a breaking change so anything goes (?)

bholloway commented 6 years ago

So I guess I am leaning toward..

mvastola commented 6 years ago

Hmmm I think so. However I think it might just be easier to just nag to set attempts explicitly.

That works too.

I have been in a situation of developing build tools for multiple teams. You don't want some configuration aspect to only surface when one of those teams is already using the tool (and makes a bad import). Better that you tell up-front.

Would that happen though? As long as we discourage setting attempts=0, the only difference is if you've been using the tool for a while you get a handy reminder that your build is about to break when you next upgrade and exactly how/where. Otherwise, if you start at 3.x.x, any strangling uncorrected paths will break just the same; previous users were just told how to fix them. That's how deprecation warnings always work.

mvastola commented 6 years ago

Think about how we migrate attempts=1 to additional npm package in 3.0.0

You mean attempts=0? As in require a secondary package in 3.0.0 for attempts=0? I'm cool with that, but that doesn't really affect this issue. I think it still makes sense to have a warning and make the message say something like "You will need an auxiliary package to maintain this recursive search functionality as of the next major release."

mvastola commented 6 years ago

FYI, just fixed this and it seems to work perfectly now.

mvastola commented 6 years ago

Anywho, @bholloway, let me know what you decide about this PR. Once this is decided, we can theoretically merge #71, and I'll put in the PR against webpacker. (I'm still of course happy to help you with #70, however.)

If we are going to go with a deprecation message, presumably we just need to decide which way #70 will be implemented first (i.e. whether or not we're going to go the npm module route) because that will influence the deprecation message here.

bholloway commented 6 years ago

Sorry at the Github event in Sydney yesterday and today, no laptop. Get back to you soon.

mvastola commented 6 years ago

Oh. Enjoy then!

bholloway commented 6 years ago

(Sorry if we have already covered this and I am just not tracking well) I am thinking we should just merge #71 as-is.

Certainly that would be better than what we have in the wild right now. It is pretty low risk for webpacker to upgrade to, and gets you want you originally wanted sooner.

I have further thoughts on the nag, etc. I sense we are not converging on this issue as quickly as first thought. It makes sense to considered it as an increment.

mvastola commented 6 years ago

I'm all for that. Especially since the default is attempts=0, there's no risk from adding the deprecation warning in a later release.

mvastola commented 6 years ago

Ok. webpacker is now merged. Happy to help you decide on this if you're at all interested.