V3SlotSetters linter/codemod draft #1669

Spone · 2023-02-28T01:03:12Z

What are you trying to accomplish?

I'd like to provide a linter / codemod to ease the upgrade to ViewComponent v3.0.0, especially the new slot setters syntax which is in my opinion the main hassle when upgrading.

For now, this draft only outputs suggestions of changes. If we're confident enough we don't have false positives, we can change it to actually modify the files (or allow running it as a linter or as a codemod).

What approach did you choose and why?

I first tried to experiment with Ruby AST / Parser-based solutions, but given that we have to access the slot names in ViewComponent classes and parse the content of various templates (erb, slim, haml), it did not appear as a viable solution.

Then, I implemented a two-steps approach:

Load all ViewComponents and read the name of their slots;
Try to find exact matches: scan all components and view files to find renders of the components, called with blocks, and methods matching the name of their slots' setter methods. These suggestions are prefixed with probably in the output ;
In some cases (such as when using helpers to render components instead of render(MyComponent.new)), this was not enough to find all occurrences so I added another "uncertain matches" step: try to match the name of the slots in all files. These suggestions are prefixed with maybe in the output.

I tested this on several projects, here is the output:

lookbook v1.4.5 which is the latest version without ViewComponent v3 support: view output and compare with the changes made manually:
- lookbook-hq/lookbook@f3cffec
- lookbook-hq/lookbook@2254901
polaris_view_components which does not support ViewComponent v3 yet: view output
avo which does not support ViewComponent v3 yet: view output
a few internal projects I'm working on (I can't share the output infortunately)

Anything you want to highlight for special attention from reviewers?

I packaged this as a class that could be included to the gem.
The exact way people can run this remains to be defined.

We also have to figure a way to test this :)

What's left to do

fix render_match
autodetect view paths
improve uncertain matches by targeting full slot names instead of partial. This should reduce false positive results
add a rake task that will run codemod
make it accessible in rails console too
add a "--fix" mode that modifies the files (similar to standardrb --fix or rubocop -A)
tests
docs
changelog

camertron · 2023-02-28T05:07:08Z

Very cool, thanks for taking this on! Before I really dive in I have a few questions.

Have you had a look at erb-lint? It can parse ERB and I think uses the Parser gem under the hood. Might be a viable way to parse and manipulate ERB code.
I know we probably want this codemod to work statically, but I'll bet we could identify slot getters/setters really accurately by setting a tracepoint and rendering all the previews + running all the tests. What do you think?

Spone · 2023-02-28T08:33:10Z

Have you had a look at erb-lint? It can parse ERB and I think uses the Parser gem under the hood. Might be a viable way to parse and manipulate ERB code.

I considered it but I wanted something that would work for any templating language (or at least the most popular ones: ERB, Slim, Haml), since I use Slim in most projects.

I know we probably want this codemod to work statically, but I'll bet we could identify slot getters/setters really accurately by setting a tracepoint and rendering all the previews + running all the tests. What do you think?

That would be an interesting approach, but it would only work on thoroughly previewed / tested components, which is sadly not always the case 🙃
I'd love to see what this would look like though! Maybe we can add this as another scanning step.

allmarkedup · 2023-03-01T03:53:16Z

@Spone this is fantastic! Really great work mate. I have been putting off the big upgrade job at work but this looks like it'll get me a lot closer. Very exciting, I'm sure that the slot name changes are a blocker for quite a few people so this looks like a great solution, even if it only ended up getting 95% of the way there.

I'm off for a couple of days but will give it a test on our codebase when I'm back. I think I'll probably still try to combine it with some sort of monkey-patched ViewComponent gem to hopefully log anywhere that old-style slot names are still being used after the codemod has run, just to be sure.

boardfish · 2023-03-01T11:25:22Z

I'm also planning to test this on our migration to ViewComponent 3 once I get around to it. Thanks for putting it together!

I really like providing codemods as a pattern to help folks along their migration path. We could tuck this inside a generator that helps folks to handle migrations, similar to Rails' own app:update task.

joelhawksley · 2023-03-08T18:34:37Z

@Spone I like it! Are you hoping to land this in a Release Candidate ahead of 3.0.0 landing?

Spone · 2023-03-08T18:57:21Z

@joelhawksley I'd love to, if I get sufficient feedback from people who tried it!

I ran this on more codebases, and noticed a few false-positives that I'd like to fix as well.

joelhawksley

I think others will find this tool helpful. Would it be worth writing a simple test for it? We don't need to be too thorough.

lib/view_component/codemods/v3_slot_setters.rb

joelhawksley · 2023-03-10T12:56:40Z

lib/view_component/codemods/v3_slot_setters.rb

+#
+#     ViewComponent::Codemods::V3SlotSetters.new.call
+#
+# If your app uses custom paths for views, you can pass them in:


Could we load these from the rails config instead of requiring them to be passed in?

lib/view_component/codemods/v3_slot_setters.rb

joelhawksley · 2023-03-10T13:00:30Z

@Spone I'm hoping to wrap up the v3 release cycle in the next week or two, so perhaps you could decide whether to land this in the next week?

boardfish · 2023-03-10T14:18:44Z

Ran this against our codebase @fac. I'd already gone through and made all the necessary changes previously on another branch.

When I ran this before I had updated the slot syntax, it came up with a lot of maybe cases. Enough of these were false positives that I can say this was quite noisy for my use case. In particular, it caught a lot of method names that begin with the names of slots that exist on components. For example, it would catch total_somethings where there was a component with a slot named total. It'd be good to rework that side of things and perhaps also make it easier to filter the output.

When I ran this after I had updated the slot syntax, it didn't turn up any cases at all, which I'm hoping suggests that I had found everything that needed to be fixed.

Ideally, I think it would be really good to use erb-lint and rubocop to do this, with a view to making it possible to autocorrect these with confidence. That being said, there are definitely "maybe" cases, especially in cases where folks have components aliased to helpers.

joelhawksley · 2023-03-28T20:31:11Z

@Spone are you still interested in landing this PR? I'd be cool either way but I'm trying to get a feel of what we'd like to land with v3 final.

Spone · 2023-03-29T19:28:38Z

@joelhawksley Sorry I missed your previous comment. I'd like to continue improving this but I cannot commit to a specific time frame. I feel like this can be part of an upcoming v3.x minor version release. What do you think?

Co-authored-by: Joel Hawksley <[email protected]>

Spone · 2023-04-26T21:01:13Z

I'd be open to pairing with someone to wrap this up!

owaiswiz · 2023-04-29T02:29:07Z

lib/view_component/codemods/v3_slot_setters.rb

+          rendered_components = []
+          content = File.read(file)
+
+          if (render_match = content.match(RENDER_REGEX))


Is this assuming only a single render_match per file?

If I render multiple different components in the same file, would this catch it?

After re-reading the code, I guess you're right. I'm a bit surprised I didn't catch this earlier 😅

This part needs to be reworked.

kirillplatonov · 2023-04-29T14:38:15Z

@Spone what's left here in order to get this ready for merge? Some suggestions I have:

Improve uncertain matches by targeting full slot names instead of partial. This should reduce false positive results
Add a rake task that will run codemod
Make it accessible in rails console too

Spone · 2023-05-04T15:36:29Z

@kirillplatonov I added a "What's left to do" section to the PR description, including your suggestions. Would you like to take care of some of these items?

kirillplatonov · 2023-05-08T09:43:16Z

@Spone sure! Should I open a PR with my changes to codemods/v3-slots branch?

Spone · 2023-05-08T10:38:05Z

Yes please do!

* Add rake task for legacy slots detection * Add view component previews to codemod view paths * Autodetect rails view paths * Improve uncertain matches detection * Update lib/view_component/codemods/v3_slot_setters.rb Co-authored-by: Hans Lemuet <[email protected]> * Update usage instruction * Add rake task to migrate to new slots * Update changelog * Standardrb * Add tests * Call eager loading via Rails * Update docs/CHANGELOG.md Co-authored-by: Hans Lemuet <[email protected]> * Add support for passing view paths via CLI * Fix helper in rake tasks * Fix tests * Add note about codemod to changelog --------- Co-authored-by: Hans Lemuet <[email protected]>

joelhawksley

Might we add something to the docs for this?

Spone · 2023-07-06T20:30:10Z

Might we add something to the docs for this?

Yes, but not sure where. We don't yet have a "Upgrading" page.

Also, we need to fix the changelog, the entry has to be under the main heading.

joelhawksley · 2023-07-06T21:30:28Z

Yes, but not sure where. We don't yet have a "Upgrading" page.

How about adding a note to the Slots docs?

Twistedben · 2023-07-09T17:15:19Z

Hey guys!

I started working on a similar solution last week and had no idea! At eXp-Realty we use a lot of View Components and have 10+ production apps with them in them. I built a Ruby script to automatically update these slot calls with the with_ change. It's amazing how similar our solutions are though I think yours is better. I just wanted to add to the discussion and my script file if it can help at all. In one of our apps that I used it, it correctly updated over 327 slots.

https://github.com/Twistedben/view_component_3_slot_upgrade

camertron · 2023-07-26T18:17:43Z

Hey @Spone, how's this PR coming along? Is it ready to merge?

Spone · 2023-08-09T20:52:09Z

Haven't had time to look at this again before holidays, I'll get back to it when I'm back to work. Thanks everyone for your patience 🙏

JWShuff · 2023-12-20T19:16:51Z

Hey guys!

I started working on a similar solution last week and had no idea! At eXp-Realty we use a lot of View Components and have 10+ production apps with them in them. I built a Ruby script to automatically update these slot calls with the with_ change. It's amazing how similar our solutions are though I think yours is better. I just wanted to add to the discussion and my script file if it can help at all. In one of our apps that I used it, it correctly updated over 327 slots.

https://github.com/Twistedben/view_component_3_slot_upgrade

Just used that script to great effect, TY! Noted a small issue with encoding and have opened a PR against your repo if you'd like.

joelhawksley · 2024-08-05T20:00:08Z

@Spone how do you think we should proceed with this PR?

Twistedben · 2024-08-05T21:12:51Z

Hey guys!
I started working on a similar solution last week and had no idea! At eXp-Realty we use a lot of View Components and have 10+ production apps with them in them. I built a Ruby script to automatically update these slot calls with the with_ change. It's amazing how similar our solutions are though I think yours is better. I just wanted to add to the discussion and my script file if it can help at all. In one of our apps that I used it, it correctly updated over 327 slots.
https://github.com/Twistedben/view_component_3_slot_upgrade

Just used that script to great effect, TY! Noted a small issue with encoding and have opened a PR against your repo if you'd like.

Thanks @JWShuff ! I'm glad it worked well for you. I merged your PR on my script

reeganviljoen · 2024-08-08T06:52:41Z

@Spone I would really love to see this go through, if you need my help in getting it over the line please ask me as I would love to help if its needed

V3SlotSetters codemod draft

de176b5

Spone added the v3 label Feb 28, 2023

Spone self-assigned this Feb 28, 2023

Spone referenced this pull request in lookbook-hq/lookbook Feb 28, 2023

Use with_ prefix for slots

f3cffec

Spone requested review from BlakeWilliams, camertron, joelhawksley, allmarkedup and boardfish February 28, 2023 01:46

Spone mentioned this pull request Feb 28, 2023

Support for ViewComponent v3 baoagency/polaris_view_components#277

Closed

joelhawksley approved these changes Mar 10, 2023

View reviewed changes

boardfish mentioned this pull request Mar 27, 2023

Test that all previews render correctly, e.g. in CI lookbook-hq/lookbook#376

Closed

Spone and others added 6 commits March 29, 2023 21:51

Merge branch 'main' into codemods/v3-slots

c413492

Update lib/view_component/codemods/v3_slot_setters.rb

21a8a26

Co-authored-by: Joel Hawksley <[email protected]>

Update lib/view_component/codemods/v3_slot_setters.rb

fb748a6

Co-authored-by: Joel Hawksley <[email protected]>

Merge branch 'main' into codemods/v3-slots

d075cde

standardrb

e376b29

Add Zeitwerk eager loading to initializer

547c202

Merge branch 'main' into codemods/v3-slots

2542ba5

Spone added help wanted slots Related to the Slots feature labels Apr 26, 2023

owaiswiz reviewed Apr 29, 2023

View reviewed changes

Merge branch 'main' into codemods/v3-slots

b805fd7

kirillplatonov mentioned this pull request May 8, 2023

Codemod v3 slots improvements #1746

Merged

Spone marked this pull request as ready for review June 29, 2023 20:54

Merge branch 'main' into codemods/v3-slots

cf8a038

Spone requested a review from joelhawksley June 29, 2023 20:55

joelhawksley approved these changes Jul 6, 2023

View reviewed changes

boardfish mentioned this pull request Nov 28, 2023

Add more strict process around breaking changes #1918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V3SlotSetters linter/codemod draft #1669

V3SlotSetters linter/codemod draft #1669

Spone commented Feb 28, 2023 •

edited

Loading

camertron commented Feb 28, 2023

Spone commented Feb 28, 2023 •

edited

Loading

allmarkedup commented Mar 1, 2023

boardfish commented Mar 1, 2023

joelhawksley commented Mar 8, 2023

Spone commented Mar 8, 2023

joelhawksley left a comment

joelhawksley Mar 10, 2023

joelhawksley commented Mar 10, 2023

boardfish commented Mar 10, 2023

joelhawksley commented Mar 28, 2023

Spone commented Mar 29, 2023

Spone commented Apr 26, 2023 •

edited

Loading

owaiswiz Apr 29, 2023

Spone May 4, 2023

kirillplatonov commented Apr 29, 2023 •

edited

Loading

Spone commented May 4, 2023

kirillplatonov commented May 8, 2023

Spone commented May 8, 2023

joelhawksley left a comment

Spone commented Jul 6, 2023

joelhawksley commented Jul 6, 2023

Twistedben commented Jul 9, 2023

camertron commented Jul 26, 2023

Spone commented Aug 9, 2023

JWShuff commented Dec 20, 2023

joelhawksley commented Aug 5, 2024

Twistedben commented Aug 5, 2024

reeganviljoen commented Aug 8, 2024

V3SlotSetters linter/codemod draft #1669

Are you sure you want to change the base?

V3SlotSetters linter/codemod draft #1669

Conversation

Spone commented Feb 28, 2023 • edited Loading

What are you trying to accomplish?

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

What's left to do

camertron commented Feb 28, 2023

Spone commented Feb 28, 2023 • edited Loading

allmarkedup commented Mar 1, 2023

boardfish commented Mar 1, 2023

joelhawksley commented Mar 8, 2023

Spone commented Mar 8, 2023

joelhawksley left a comment

Choose a reason for hiding this comment

joelhawksley Mar 10, 2023

Choose a reason for hiding this comment

joelhawksley commented Mar 10, 2023

boardfish commented Mar 10, 2023

joelhawksley commented Mar 28, 2023

Spone commented Mar 29, 2023

Spone commented Apr 26, 2023 • edited Loading

owaiswiz Apr 29, 2023

Choose a reason for hiding this comment

Spone May 4, 2023

Choose a reason for hiding this comment

kirillplatonov commented Apr 29, 2023 • edited Loading

Spone commented May 4, 2023

kirillplatonov commented May 8, 2023

Spone commented May 8, 2023

joelhawksley left a comment

Choose a reason for hiding this comment

Spone commented Jul 6, 2023

joelhawksley commented Jul 6, 2023

Twistedben commented Jul 9, 2023

camertron commented Jul 26, 2023

Spone commented Aug 9, 2023

JWShuff commented Dec 20, 2023

joelhawksley commented Aug 5, 2024

Twistedben commented Aug 5, 2024

reeganviljoen commented Aug 8, 2024

Spone commented Feb 28, 2023 •

edited

Loading

Spone commented Feb 28, 2023 •

edited

Loading

Spone commented Apr 26, 2023 •

edited

Loading

kirillplatonov commented Apr 29, 2023 •

edited

Loading