search

unabridged / motion

Reactive frontend UI components for Rails in pure Ruby
https://github.com/unabridged/motion
MIT License
697 stars 19 forks source link

Add ViewComponent >2.35.0 vars to the exclusion list #113

Closed r3trofitted closed 2 years ago

r3trofitted commented 3 years ago

This PR should fix the problems with ViewComponent 2.35.0 (see #111).

Unfortunately, I couldn't find a way to add tests for this fix. Instead, I've just reviewed the list of newly prefixed variables and added those that were already in the Rendering module under their former name.

It's a bit rough, but as far as I can tell, it works.

jbbarth commented 3 years ago

@r3trofitted don't you think we could filter out any instance variable starting with @__vc_ ? I think ViewComponent intends to prefix instance variables private to their implementation with this. So something more generic would be more future proof ? Or maybe you think it's too complicated/risky ?

latortuga commented 3 years ago

@r3trofitted thanks for the contribution!

I agree with @jbbarth that filtering by the variable prefix seems like a great strategy. Can you update this to try that?

r3trofitted commented 3 years ago

@latortuga Sure, here you go!

To be honest, I'm a bit on the fence about this strategy, though. Keeping a registry of variables to be excluded is tedious and brittle, but since I know very little about both Motion and View Component, I was feeling more confortable staying close to the existing logic. But if you're confident that Motion will never ever need to include one of VC's “private” variables in the dump, then, filtering on the prefix is definitely a better approach. I leave it to you!

latortuga commented 3 years ago

I've been noodling on this for a few days and I can't decide what I think. @alecdotninja or @Gmfholley can you weigh in on this?