-
-
Notifications
You must be signed in to change notification settings - Fork 132
Debug mode: Never wrap head-emitting helper output in debug spans #1953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
379d027
36c27e9
5ce00a6
fd42478
53811bd
a27c27b
d949a5b
a2b2588
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,14 @@ | ||
| # frozen_string_literal: true | ||
| # typed: false | ||
|
|
||
| require_relative "../action_view/helper_registry" | ||
|
|
||
| module Herb | ||
| class Engine | ||
| class DebugVisitor < Herb::Visitor | ||
| HEAD_CONTENT_HELPER_PATTERN = /\b(?:#{Herb::ActionView::HelperRegistry.head_content_helpers.map { |helper| Regexp.escape(helper.name) }.join("|")})\b/ #: Regexp | ||
| private_constant :HEAD_CONTENT_HELPER_PATTERN | ||
|
|
||
| def initialize(file_path: nil, project_path: nil) | ||
| super() | ||
|
|
||
|
|
@@ -76,7 +81,7 @@ def visit_erb_content_node(node) | |
| if !@in_attribute && !@in_html_comment && !@in_html_doctype && !in_excluded_context? && erb_output?(node.tag_opening.value) | ||
| code = node.content.value.strip | ||
|
|
||
| @erb_nodes_to_wrap << node unless complex_rails_helper?(code) | ||
| @erb_nodes_to_wrap << node unless complex_rails_helper?(code) || head_content_helper?(code) | ||
| end | ||
|
|
||
| super | ||
|
|
@@ -210,7 +215,7 @@ def create_debug_span_for_erb(erb_node) | |
| code = erb_node.content.value.strip | ||
| erb_code = "#{opening} #{code} %>" | ||
|
|
||
| return erb_node if complex_rails_helper?(code) | ||
| return erb_node if complex_rails_helper?(code) || head_content_helper?(code) | ||
|
|
||
| line = erb_node.location&.start&.line | ||
| column = erb_node.location&.start&.column | ||
|
|
@@ -329,7 +334,7 @@ def in_script_or_style_context? | |
| end | ||
|
|
||
| def in_excluded_context? | ||
| excluded_tags = ["script", "style", "head", "title", "textarea", "pre", "svg", "math"] | ||
| excluded_tags = ["script", "style", "head", "title", "meta", "link", "base", "textarea", "pre", "svg", "math"] | ||
| return true if excluded_tags.any? { |tag| @element_stack.include?(tag) } | ||
|
|
||
| if @erb_block_stack.any? { |node| javascript_tag?(node.content.value.strip) || include_debug_disable_comment?(node.content.value.strip) } | ||
|
|
@@ -386,6 +391,17 @@ def javascript_tag?(code) | |
| false | ||
| end | ||
|
|
||
| # TODO: Rewrite using Prism Nodes once available | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this TODO because related methods have this same TODO |
||
| def head_content_helper?(code) | ||
| cleaned_code = code.strip.gsub(/\s+/, " ") | ||
|
|
||
| return true if cleaned_code.match?(HEAD_CONTENT_HELPER_PATTERN) | ||
|
|
||
| return true if cleaned_code.match?(/\btag\.(?:meta|link|title|base)\b/) | ||
|
|
||
| false | ||
| end | ||
|
|
||
| def include_debug_disable_comment?(code) | ||
| cleaned_code = code.strip.gsub(/\s+/, " ") | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kcdragon I think this is the best thing to do long-term!
Maybe we could reverse it to something like:
So other helpers could define other contexts, like:
There is also this other pull request which adds more schema-like metadata that could be also useful to have: #1030
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcoroth Cool. I'll change this PR to use
context: "head". The other PR is interesting. Is the idea that if we knew which element these helpers outputted, we could automatically determine if it could only appear in<head>without needing something likecontext: "head"?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, at least as long as the helper actually maps to an HTML tag.