use do rather than require to load a mojo application #2097
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
kraih
requested review from
a team,
marcusramberg,
kraih,
jhthorsen and
christopherraa
The return value of require is usually not a reasonable thing to rely on, aside from it being true. The first time requiring a file, it will return the value of the last statement in the file. The second time requiring a file, it will return a simple true value. Mojo was bypassing this problem by deleting the %INC entry for the file, forcing it to always be loaded again. But the new module_true core feature will cause require to always return a simple true value rather than the last statement in the file. This does not apply to do though. Switch to using do to be compatible with code using the new feature.
haarg
force-pushed
the
fix-module-true-compat
branch
from
0cc3b20 to
693fc6c
Compare
jberger
approved these changes
Aug 24, 2023
There was a problem hiding this comment.
Generally I approve, however I wonder if in the test it should explicitly load the module_true pragma rather than the feature bundle, and if it should do so after Mojolicious::Lite. That way if some "fixup" is ever attempted in either of those places we'd still be explicitly testing the pragma not the generic scenario? Maybe both?
jhthorsen
approved these changes
Aug 25, 2023
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.
Summary
Use
dorather thanrequireto load a mojo application to fix compatibility with newmodule_truecore feature.Motivation
The return value of
requireis usually not a reasonable thing to rely on, aside from it being true. The first time requiring a file, it will return the value of the last statement in the file. The second time requiring a file, it will return a simple true value.Mojo was bypassing this problem by deleting the
%INCentry for the file, forcing it to always be loaded again. But the newmodule_truecore feature will causerequireto always return a simple true value rather than the last statement in the file. This does not apply todothough.References
#2094