Skip to content
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

Serialize view components #66

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rickychilcott
Copy link
Contributor

@rickychilcott rickychilcott commented Oct 10, 2020

WIP Serialize View Component Spike

Description

This is a proof of concept PR that shows we can shim into the ViewComponent initialization process, capture the raw arguments, and then serialize them back out.

There are limitations here in that if someone modifies some state of the component after initialization, it won't be reflected. I'm not certain that's typical (or recommended), but it could create some confusion. That said, people who do that could override the #to_futurism_serialized method to control what gets included and passed back in. Nobody should ever do that though :)

If this general approach works, I'd like to ask the ViewComponent team if they could own storing off the raw initialization arguments and then we can just write our own serializer. Alternatively, the ViewComponent team could consider adding serializing support view ActiveModel::Serialization

Addresses #36

This PR is for discussion and just one portion of supporting ViewComponents via futurism.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@rickychilcott rickychilcott marked this pull request as draft October 10, 2020 23:49
@rickychilcott
Copy link
Contributor Author

rickychilcott commented Oct 11, 2020

Regarding serializing collections, that should be much easier. ViewComponent::Collection has an initializer which we can implement a to_futurism_serialized method wrapping up @component, @collection, and @options.

Given the various forms of ViewComponent collections and instances, along with our already supported partials, I wonder if we should wrap these all up into a common PORO format for serialization to make deserialization clearer.

@rickychilcott
Copy link
Contributor Author

@julianrubisch do you have any thoughts on this idea?

@julianrubisch
Copy link
Contributor

Sorry, I‘m a bit swamped in work here. I will review this ASAP

@rickychilcott
Copy link
Contributor Author

All good. I've thought about this a bit more and here are some ideas:

  1. This could be improved by not even really overriding the new method, but instead just serializing all instance variables upon serialization, and then providing a mechanism to re-hydrate a view component from those instance variables. Easier to code up a solution than describe it.

  2. The biggest failure of this PR and really the biggest unknown with serializing view components, is whether we can serialize the blocks of code that could get passed in. eg:

<%= render(ModalComponent.new) do |component| %>
  <% component.with(:header) do %>
      Hello Jane
    <% end %>
  <% component.with(:body) do %>
    <p>Have a great day.</p>
  <% end %>
<% end %>

If that's not possible, and I'm honestly not sure it is, then I think we're stuck and #36 can never happen.

class ComplexArgumentComponent < ViewComponent::Base
def initialize(active, path, decativate: "Deactivate", activate: "Activate", **others)
@active, @path = active, path
@deactivate = decativate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo `

Suggested change
@deactivate = decativate
@deactivate = deactivate

end

class ComplexArgumentComponent < ViewComponent::Base
def initialize(active, path, decativate: "Deactivate", activate: "Activate", **others)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Suggested change
def initialize(active, path, decativate: "Deactivate", activate: "Activate", **others)
def initialize(active, path, deactivate: "Deactivate", activate: "Activate", **others)


def self.new(*arguments, &block)
instance = allocate
instance.singleton_class.include(FuturismSerializer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the same as instance.extend(FuturismSerializer) ?

Adds to obj the instance methods from each module given as a parameter.

@julianrubisch
Copy link
Contributor

Finally got to taking a look at this. This is a beautiful solution!

I agree with your concerns about VC's content_areas/slots etc. Admittedly, the API is stable but still quite in flux and I'm somewhat fearful that we'll inherit their bugs when things change.

One approach could always be to announce limited support of VCs for the time being. What do you think?

About submitting a PR to ViewComponent proper: You have my definite consent for continuing, but even if that were successful, we'd have to depend on a future release of VC, correct?

@rickychilcott
Copy link
Contributor Author

Sorry for not responding. Last week was my first week back to work after being out for 2 months. I may be slow(er) to respond in the coming weeks.

I think some limited support for VCs could at least get the conversation going and could be valuable. Particularly if in the development environment, we raise a warning if they use the slots or content as not supported.

RE ViewComponent carrying some of this, yes your assumption is correct. They are releasing quite quickly though and I wouldn't imagine that too many futurism users who need/want VC support would bulk at making taking the small step to jump to the latest VC release.

@adambedford
Copy link

Hi @rickychilcott @julianrubisch Have you got this working in production at all?

@julianrubisch
Copy link
Contributor

Sadly, no. We concluded that it'd be more clever to wait for VC 3 to come out...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants