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

Refactor element - use SGIDs where possible to minimize over-the-wire size #37

Merged
merged 6 commits into from
Aug 23, 2020

Conversation

julianrubisch
Copy link
Contributor

Refactor

Description

Clean up the internals a bit, create a wrapper class for a futurism Element

@julianrubisch julianrubisch marked this pull request as ready for review August 5, 2020 18:03
@julianrubisch julianrubisch changed the title Refactor element Refactor element - use SGIDs where possible to minimize over-the-wire size Aug 5, 2020
@leastbad
Copy link
Contributor

So I have exhaustively gone over every line of this library, and looked up everything I didn't understand. I want to call out
data.fetch_values("signed_params", "sgids") { |key| [nil] }.transpose
as particularly amazing code. That's going straight into the books.

I remain incredibly impressed with the cleverness of this solution, but I am seeing some strange behaviour.

First of all, here are the parameters of my testing. I have 100 instances of a Post model, which has a title attribute generated with Faker.

class Post < ApplicationRecord
  def to_partial_path
    "home/post"
  end
end

I have two partials, post and card:

_post.html.erb:

<tr style="height: 100px">
  <td><%= post.title %></td>
</tr>

_card.html.erb:

<div><%= message %> I'm here, now!</div>

Finally, my test page looks like this:

<h1>Futurism is now</h1>
<div style="margin-bottom: 1000px">⬇️ You'd better get scrolling...⬇️</div>

<%= futurize partial: "home/card", locals: {message: "Hello"}, extends: :div, html_options: {style: "color: blue"} do %>
  <div class="spinner"></div>
<% end %>

<%= futurize "home/card", locals: {message: "Die, scum!"}, extends: :div, html_options: {style: "color: green"} do %>
  <div class="spinner"></div>
<% end %>

<h2>TRs</h2>
<table>
  <tbody>
    <%= futurize @posts, extends: :tr, html_options: {style: "color: red"} do %>
      <td class="placeholder">&nbsp;</td>
    <% end %>
  </tbody>
</table>

As I start to inch down the page with my element inspector open, I hit what I consider to be a strange scenario. I can see the TRs header, which means that the two card partials should already be loaded into the page as they are 100% inside the viewport.

I see this on my server console:

  Rendered home/_card.html.erb (Duration: 681.2ms | Allocations: 492885)
  Post Load (0.2ms)  SELECT "posts".* FROM "posts" WHERE "posts"."id" = $1 LIMIT $2  [["id", 111], ["LIMIT", 1]]
  Rendered home/_post.html.erb (Duration: 0.1ms | Allocations: 9)

and I see this in my inspector:

chrome_EYwG9F8KSm

Both of the futurism-element nodes are still present in the DOM, even though the first TR has already loaded content, as you can see from the Rendered home/_post.html.erb (Duration: 0.1ms | Allocations: 9) above. But there's no sign of either card partial.

  1. Even though "Die, scum!" is just a few characters longer than "Hello", the signed-params string is significantly longer. No wonder the basic, serialized models were coming in over 1k.
  2. Rendering one (but inexplicably, not both) card partial took 681ms and half a million allocations. WTaF? It is literally just a string.

I just tried it again:

Rendered home/_card.html.erb (Duration: 1261.3ms | Allocations: 1193126)

So yeah, something is obviously fucked up.

If I comment out the first partial block (using the partial key) I see more of the same:

Rendered home/_card.html.erb (Duration: 679.8ms | Allocations: 492982)

chrome_vFhXQ7dGOf

But, if I comment out the second partial block (string shortcut) and try again, I see:

Rendered home/_card.html.erb (Duration: 0.2ms | Allocations: 105)

chrome_7fOKpB1EEL

Success! So it currently seems like calling the string shortcut version is broken.

Now, if I try calling multiple card partials in a row, it works just fine, because the selector should always grab the next matching element on the page.

<%= futurize partial: "home/card", locals: {message: "Hello"}, extends: :div, html_options: {style: "color: blue"} do %>
  <div class="spinner"></div>
<% end %>

<%= futurize partial: "home/card", locals: {message: "Goodbye"}, extends: :div, html_options: {style: "color: purple"} do %>
  <div class="spinner"></div>
<% end %>

<h2>TRs</h2>
<table>
  <tbody>
    <%= futurize @posts, extends: :tr, html_options: {style: "color: red"} do %>
      <td class="placeholder">&nbsp;</td>
    <% end %>
  </tbody>
</table>
  Rendered home/_card.html.erb (Duration: 0.2ms | Allocations: 105)
  Rendered home/_card.html.erb (Duration: 0.0ms | Allocations: 7)

chrome_itLFfSHvwI

In conclusion, I think PR#37 is good! But I suspect that something is off with the shorthand syntax.

@julianrubisch julianrubisch merged commit 2a0db0b into master Aug 23, 2020
@julianrubisch
Copy link
Contributor Author

@all-contributors add @leastbad for code and review

@allcontributors
Copy link
Contributor

@julianrubisch

I've put up a pull request to add @leastbad! 🎉

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.

2 participants