Don't mutate bundler dependencies in place #433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`Bundler.require` returns the set of dependencies bundler definition is considering for the current invocation. And those dependencies were being filtered and the ones outside of plugins' groups removed (`puma` is usually one of these). `Bundler.definition.specs` materializes the current dependencies into a set of requirable specifications. However, since some dependencies had been wiped out after the plugin manager had done its thing, the corresponding specs would no longer be materialized and would be missing. This commit fixes the problem by not mutating bundler dependencies in-place.
|
Thanks @deivid-rodriguez, your explanation makes sense and is much appreciated!. And I agree keeping that other fix in place is a good call. |
|
Thank you, and keep up the good work, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a 🐛 bug fix, although a workaround is already in place (#430) and in my opinion it should not be reverted because it's a more bullet proof approach. So this fix is not very important, but given the amount of
bundlerinternals Bridgetown seems to use, it might save you & us from future trouble!Summary
It was reported to us a question about why your previous check of whether
pumawas present in the bundle (throughBundler.definition.specs) wouldn't work while the new one (through directly requiring it and rescuingLoadError) would work.I investigated the root cause thanks to a helpful reproduction provided in the issue, and I found the culprit in
bridgetown's plugin manager mutating the array of bundler dependencies and causing this issue as a side effect.Plugin manager uses
Bundler.require, which returns the set of dependencies bundler definition is considering for the current invocation. And those dependencies were being filtered and the ones outside of plugins' groups removed (pumais usually one of these).Bundler.definition.specsmaterializes the current dependencies into a set of requirable specifications. However, since some dependencies had been wiped out by the plugin manager, the corresponding specs would no longer be materialized and would be missing.This commit fixes the problem by not mutating bundler dependencies in-place.
Context
My guess is that this might've worked in the past, but some changes in
bundlermade the issue surface. At some point requiringbundler/setupprobably memoized the set of materialized specs, so the call toBundler.definition.specswouldn't be affected by any change in the underlying dependencies. I added a change inbundlerto make sure the set of specs initially materialized by requiringbundler/setupis memoized, so that this kind of issue cannot reaper in other ways.