search

shakacode / shakapacker

Use Webpack to manage app-like JavaScript modules in Rails
MIT License
400 stars 89 forks source link

feat!: remove `globalMutableWebpackConfig` #439

Closed G-Rath closed 3 months ago

G-Rath commented 3 months ago

Summary

This was deprecated in v7 in favor of generateWebpackConfig, so now we can remove it

Pull Request checklist

ahangarha commented 3 months ago

@G-Rath @tomdracz May I know the background of this PR? Was is discussed anywhere?

G-Rath commented 3 months ago

@ahangarha I thought it was always intended to remove this and that its use is discouraged - it wasn't as strongly worded as other deprecations, but unless you think there's a solid usecase where generateWebpackConfig can't be used I'd say we stick with removing it because it's not a breaking change to add it back later if it does turn out to be worth keeping and mutating globals is generally considered a very risky thing

ahangarha commented 3 months ago

When we made this change last year, we had a long discussion on how and why to implement it this way. I have no objection for its removal. Just I think Justin should know about this change. If he was not involved in the process, let's share it with him too.

tomdracz commented 3 months ago

Plus 1 to @G-Rath comment here, I've assumed the deprecation was the desire here. I can't see a clear use case for the object being required but happy to be convinced!

When we made this change last year, we had a long discussion on how and why to implement it this way.

@ahangarha was this discussion anywhere in this repo or elsewhere? Haven't seen anything so that might be some context missing (mis)guided me here to merge in the PR

ahangarha commented 3 months ago

was this discussion anywhere in this repo or elsewhere?

No. We had a video call.

Let's just share this with Justin before it become harder to revert (in case...).