Ignoring vendor directory considered harmful#1508
Open
joelhawksley wants to merge 1 commit intomarcoroth:mainfrom
Open
Ignoring vendor directory considered harmful#1508joelhawksley wants to merge 1 commit intomarcoroth:mainfrom
joelhawksley wants to merge 1 commit intomarcoroth:mainfrom
Conversation
By default, Herb currently ignores the `vendor` directly. This is potentially quite harmful, as it could expose consuming applications to runtime exceptions. For example. We vendor https://github.com/primer/view_components in our application. The dependency includes ViewComponents with ERB files, including a few that will not compile with Herb. If we don't compile the templates in our test environment, those Herb compilation errors will not be caught. I see similar risks for other gems that provide UI, such as as dashboards mounted via an engine. We thankfully discovered this issue and manually added `vendor/**/*.erb` to our `include` configuration, but I'm wary of blankly excluding the directory silently by default, especially when we describe the behavior of `herb analyze` as `Running herb analyze without arguments now defaults to the current directory`. This could make one believe that all subdirectories are being analyzed based on the config in the user's `.herb.yml` file, but in fact it is the combination of `defaults.yml` and the user's configuration. Signed-off-by: Joel Hawksley <joelhawksley@github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
By default, Herb currently ignores the
vendordirectly. This is potentially quite harmful, as it could expose consuming applications to runtime exceptions.For example. We vendor https://github.com/primer/view_components in our application. The dependency includes ViewComponents with ERB files, including a few that will not compile with Herb. If we don't compile the templates in our test environment, those Herb compilation errors will not be caught and would raise in production.
I see similar risks for other gems that provide UI, such as as dashboards mounted via an engine.
I'm wary of blankly excluding the directory silently by default, especially when we describe the behavior of
herb analyzeasRunning herb analyze without arguments now defaults to the current directory(https://herb-tools.dev/blog/whats-new-in-herb-v0-9#improved-herb-analyze-command). This could make one believe that all subdirectories are being analyzed based on the config in the user's.herb.ymlfile, but in fact it is the combination ofdefaults.ymland the user's configuration.It also appears that adding
'vendor/**/*.erb'toincludestill results in the directory being excluded. I had to explicitly callbundle exec herb analyze vendorto see errors from the vendor folder!Of course, this does nothing to avoid similar issues for gems that provide UI that are not vendored. I believe we should include a boot-time safe-guard for this issue in ReActionView at the very least. Or, we could look into ways of not using Herb to compile ERB from gems, but that sounds like a mess to me.