Replace dependency tracker with ripper based tracker by HParker · Pull Request #42458 · rails/rails

HParker · 2021-06-11T19:09:13Z

Instead of using the ERBTracker, we can use RipperTracker which is extracted from https://github.com/jhawthorn/actionview_precompiler.

Using a parser finds dependencies that would otherwise be difficult to find with the regular expressions. It should also theoretically work with other template systems since it operates on the compiled template instead of the contents of the file.

Co-authored-by: John Hawthorn

zzak · 2021-06-14T00:40:51Z

actionview/test/template/dependency_tracker_test.rb

Was there a reason these changed? 🤔

Yes! we correctly skip commented out render calls now! 🎉 so to make this test pass it has to be a real render call.

actionview/test/template/dependency_tracker_test.rb

zzak · 2021-06-14T00:41:51Z

actionview/test/template/dependency_tracker_test.rb

Does the old form still work? Or was this showing a new feature with object kwarg?

render @pokémon, partial: 'ピカチュウ' is actually invalid render syntax. I tested it and it actually renders the pokémon template, not the ピカチュウ partial, so I updated the test to hopefully have the same intent.

zzak · 2021-06-14T00:46:19Z

Nice work here, I just have a couple simple questions to help me gain more context on this. Thanks for putting it together!!

HParker · 2021-06-14T15:55:58Z

Thanks @zzak! helpful comments. I also saw a little stray whitespace in the test file, so I am going to remove that quickly.

rafaelfranca

Just a few comments on the code organization but looks good to me otherwise.

rafaelfranca · 2021-06-24T17:37:49Z

actionview/lib/action_view/dependency_tracker.rb

Move this to before the initialize and move the initialize back to after all the class methods.

actionview/lib/action_view/dependency_tracker.rb

rafaelfranca · 2021-06-24T17:39:28Z

actionview/lib/action_view/dependency_tracker.rb

Suggested change

require "action_view/ripper_ast_parser"

rafaelfranca · 2021-06-24T17:40:10Z

actionview/lib/action_view/dependency_tracker.rb

Just put the attr_reader after the private block definition above.

rafaelfranca · 2021-06-24T17:42:48Z

actionview/lib/action_view/render_parser.rb

Suggested change

actionview/lib/action_view/render_parser.rb

rafaelfranca · 2021-06-24T17:43:45Z

actionview/lib/action_view/ripper_ast_parser.rb

Suggested change

class Node < ::Array

class Node < ::Array # :nodoc:

rafaelfranca · 2021-06-24T17:43:53Z

actionview/lib/action_view/ripper_ast_parser.rb

Suggested change

class NodeParser < ::Ripper

class NodeParser < ::Ripper # :nodoc:

rafaelfranca · 2021-06-24T17:43:59Z

actionview/lib/action_view/ripper_ast_parser.rb

Suggested change

class RenderCallExtractor < NodeParser

class RenderCallExtractor < NodeParser # :nodoc:

HParker · 2021-06-24T19:53:47Z

Ah, one thing before this merges. It looks like jbuilder uses ERBTracker, so I think I need to keep ERBTracker as an option to prevent breaking new apps. I'll also see how hard it would be to have jbuilder use the new tracker.

rafaelfranca · 2021-06-24T20:28:19Z

Ah, one thing before this merges. It looks like jbuilder uses ERBTracker, so I think I need to keep ERBTracker as an option to prevent breaking new apps. I'll also see how hard it would be to have jbuilder use the new tracker.

👍🏽. Yeah, let's keep both while we change jbuilder.

HParker · 2021-06-24T20:51:02Z

actionview/lib/action_view/dependency_tracker.rb

I feel weird requiring this here since it isn't used, but I have to have ERBTracker act like it did before this change. Let me know if that assumption is wrong.

rafaelfranca · 2021-06-24T20:52:31Z

actionview/lib/action_view/dependency_tracker.rb

We can register an autoload instead in action_view.rb

That is way better! thanks!

rafaelfranca · 2021-06-24T20:56:36Z

actionview/lib/action_view.rb

Should it be inside eager_autoload given it is not used?

🤦 It should not. Thanks!

actionview/lib/action_view.rb

HParker · 2021-06-25T17:34:12Z

I think this branch is ready! Thanks for all the amazing feedback!

I opened a PR to JBuilder to avoid their dependency on ERBTracker.

I might take a look at haml and slim to make sure they don't have any issues. Ideally the ripper tracker is flexible enough that it can handle ERB, haml and slim since it looks at the compiled source rather than the source code, but if they customize the dependency tracker like JBuilder they might need an update in the meantime.

Matt-Yorkley · 2021-07-02T18:36:54Z

Hi @HParker 👋 the RipperTracker looks great ❤️

I'm looking at resolving compatibility problems with Fragment Caching and ViewComponent, so I'm really interested in updates to ActionView::ERBTracker.

Do you have any thoughts on potentially adding some explicit support for tracking render dependencies that involve passing classes as arguments to render, eg: <%= render Example::Component.new %>?

I'll take a deeper look at the Ripper soon and maybe try to propose something more concrete, but I'm thinking along the lines of the ActionView::Template::Renderable where the identifier can be a class name instead of the usual virtual path format. Basically corresponding to this line in ActionView::Rendering where the primary argument is an arbitrary object that responds to #render_in.

Matt-Yorkley · 2021-07-02T19:05:06Z

actionview/lib/action_view/dependency_tracker/ripper_tracker.rb

Are there performance implications in compiling the template here? Or to put it another way; what are the benefits in compiling the output? Is it just so we can skip parsing render calls in commented-out lines?

There are performance implications, but it should be worth it. Most of this work is already done at boot time. One advantage to structuring the tracker this way is that it should work for all render handlers/types since Ripper can treat them all the same after they are compiled. This should mean slim, haml and any other format you might want will work accurately without modification. The most important benefit is increased accuracy. There are commented out renders and there are other ways ERBTracker could be mistakes about what the dependencies of a template are. This should avoid those problems.

Most of this work is already done at boot time.

That's true in https://github.com/jhawthorn/actionview_precompiler but is it true here? Won't the files be lazy-loaded when a cache block is encountered (and a digest is needed) during rendering, as opposed to everything being precompiled at boot time? 🤔

There are performance implications, but it should be worth it.

Is it worth doing a bit of profiling and benchmarking?

@Matt-Yorkley I think you are right about that case for the first time that that template is read or when the details change. After that, Rails should already know the digest. I am going to work on some benchmarks anyway since I should have posted those before regardless.

We should also be ready to move more template digesting to boot time now which should be better regardless since that should be faster no matter how much slower ripper is compared to ERB tracker.

Matt-Yorkley · 2021-07-03T12:54:57Z

actionview/lib/action_view/render_parser.rb

Is this superfluous after the previous check on line 99, given that RENDER_TYPE_KEYS is a subset of ALL_KNOWN_KEYS? Or am I reading it wrong?

Ah, this logic is confusing. The previous check if (keys & RENDER_TYPE_KEYS).size < 1 checks that you have at least one render key. This check unless (keys - ALL_KNOWN_KEYS).empty? checks that you don't have any keys not listed in ALL_KNOWN_KEYS. I am going to try if (keys - ALL_KNOWN_KEYS).any? since unless _.empty? is super backwards sounding. Thanks for catching this!

Aaaaahhh I see! I had to read it a couple more times 😅

Maybe extracting some comment-methods would be useful in this case? Like:

return nil if no_render_keys?(keys) return nil if unknown_keys?(keys)

actionview/lib/action_view/render_parser.rb

Instead of using the ERBTracker, we can use RipperTracker which is extracted from https://github.com/jhawthorn/actionview_precompiler. Using a parser finds dependencies that would otherwise be difficult to find with the regular expressions. It should also theoretically work with other template systems since it operates on the compiled template instead of the contents of the file. Co-authored-by: John Hawthorn <[email protected]>

HParker · 2021-07-12T17:40:14Z

Thanks for the comments @Matt-Yorkley,

Do you have any thoughts on potentially adding some explicit support for tracking render dependencies that involve
passing classes as arguments to render, eg: <%= render Example::Component.new %>?

I think it would probably be a lot of work to get something like that to work well with dependency resolution, but I haven't thought a lot about it. This code is already kinda slow and complicated, so my default answer is probably adding new features for a gem that isn't a part of rails is probably not a good idea. That being said I am open to being convinced I am wrong! If you want to link a thread about it on the rails forums I would love to see the discussion. We probably shouldn't discuss a new feature request/idea here.

rails-bot bot added the actionview label Jun 11, 2021

zzak reviewed Jun 14, 2021

View reviewed changes

actionview/test/template/dependency_tracker_test.rb Outdated Show resolved Hide resolved

zzak reviewed Jun 14, 2021

View reviewed changes

HParker force-pushed the use-ripper-dependency-tracker branch from e053119 to 60b707b Compare June 14, 2021 15:56

HParker force-pushed the use-ripper-dependency-tracker branch 2 times, most recently from 5c7ccb6 to ede2795 Compare June 24, 2021 16:59

rafaelfranca reviewed Jun 24, 2021

View reviewed changes

HParker force-pushed the use-ripper-dependency-tracker branch 3 times, most recently from a647829 to 6f6c8f2 Compare June 24, 2021 18:39

HParker force-pushed the use-ripper-dependency-tracker branch 2 times, most recently from c0d0618 to c5e9db9 Compare June 24, 2021 20:46

HParker commented Jun 24, 2021

View reviewed changes

rafaelfranca reviewed Jun 24, 2021

View reviewed changes

HParker force-pushed the use-ripper-dependency-tracker branch from c5e9db9 to fcbca92 Compare June 24, 2021 20:55

rafaelfranca reviewed Jun 24, 2021

View reviewed changes

HParker force-pushed the use-ripper-dependency-tracker branch from fcbca92 to 0c5ed38 Compare June 24, 2021 21:00

HParker commented Jun 24, 2021

View reviewed changes

actionview/lib/action_view.rb Outdated Show resolved Hide resolved

HParker force-pushed the use-ripper-dependency-tracker branch from 0c5ed38 to 40827d9 Compare June 24, 2021 21:38

HParker mentioned this pull request Jun 25, 2021

Remove reliance on ERBTracker from rails rails/jbuilder#504

Merged

Matt-Yorkley reviewed Jul 2, 2021

View reviewed changes

Matt-Yorkley reviewed Jul 3, 2021

View reviewed changes

Matt-Yorkley reviewed Jul 4, 2021

View reviewed changes

actionview/lib/action_view/render_parser.rb Outdated Show resolved Hide resolved

HParker force-pushed the use-ripper-dependency-tracker branch 2 times, most recently from f516e5b to cc74240 Compare July 12, 2021 17:24

HParker force-pushed the use-ripper-dependency-tracker branch from cc74240 to 897b9bf Compare July 12, 2021 17:33

Matt-Yorkley mentioned this pull request Jul 23, 2021

Fragment Caching for Renderables #42850

Closed

jhawthorn merged commit c44d702 into rails:main Aug 4, 2021

Matt-Yorkley mentioned this pull request Aug 8, 2021

Create shared examples for dependency tracker tests #42969

Merged

This was referenced Dec 14, 2024

Cache digest bug? #21951

Closed

Dependency Tracker finds dependencies in HTML tags and incorrectly in render calls #23536

Closed

joelhawksley mentioned this pull request Feb 11, 2026

Add first class component cache ViewComponent/view_component#2126

Open

	class NodeParser < ::Ripper
	class NodeParser < ::Ripper # :nodoc:

	class RenderCallExtractor < NodeParser
	class RenderCallExtractor < NodeParser # :nodoc:

Conversation

HParker commented Jun 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzak commented Jun 14, 2021

Uh oh!

HParker commented Jun 14, 2021

Uh oh!

rafaelfranca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HParker commented Jun 24, 2021

Uh oh!

rafaelfranca commented Jun 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HParker commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matt-Yorkley commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matt-Yorkley Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matt-Yorkley Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HParker commented Jul 12, 2021

Uh oh!

HParker commented Jun 25, 2021 •

edited

Loading

Matt-Yorkley commented Jul 2, 2021 •

edited

Loading

Matt-Yorkley Jul 2, 2021 •

edited

Loading

Matt-Yorkley Jul 13, 2021 •

edited

Loading