edisonywh / backoffice

Admin tool built with the PETAL stack
MIT License
217 stars 15 forks source link

Reference (don't inject) app.js and app.css in layout #49

Closed NduatiK closed 3 years ago

NduatiK commented 3 years ago

A few days ago, when we fixed purging I noticed a huge increase in the page load speeds. Turns out we had been sending 3mb in each request had a huge effect - even on localhost. Once we trimmed that to 22KB, the page loads became blazing fast (relatively at least).

As a result, I have been thinking about how else we could optimize Backoffice page loads.

Looking at Kaffy's app.html.eex it seems like they are referencing static files instead of injecting them as raw text into the base layout. This would allow caching of the css and js on the browser.

I think this would work perfectly in our case. Taking the example of one of my pages, the page loads at 193KB. Interestingly, only 42KB is the html. The other 151KB is static css and js. Reducing page sizes by nearly 80% in some cases seems worth pursuing.

edisonywh commented 3 years ago

That's strange because I definitely fixed the CSS issue with https://github.com/edisonywh/backoffice/commit/4f7a96d240a05603233677953d7c7c41acafec17, perhaps I accidentally ran the command in a stale branch at some point..

Anyway, I'm not familiar with HTML catching but that does sound like it could worth pursuing! I only did it this way because that's what LiveDashboard did, haha

NduatiK commented 3 years ago

Here is a snippet from Kaffy's app.html.eex:

<script src="/kaffy/assets/vendors/js/vendor.bundle.base.js"></script>

They are clearly doing something magical here since plug static is somehow able to reference library files. Looking at their README.md it seems like they are requiring library users to add a Plug.Static to their endpoint that references the correct asset folder for requests to /kaffy/:

plug Plug.Static,
  at: "/kaffy",
  from: :kaffy,
  gzip: false,
  only: ~w(assets)

Could we do something similar for backoffice?


I did a naive test locally and kaffy's approach seems to work pretty well.

We can require library users to add the following to their phoenix application Endpoint.ex:

  plug(Plug.Static,
    at: "/backoffice/assets",
    from: {:backoffice, "priv/static"},
    gzip: false
  )

and change the backoffice.html.eex to use this instead of raw(render(app.js)):

    <link rel="stylesheet" href="/backoffice/assets/css/app.css"/>
    <script src="/backoffice/assets/js/app.js"></script>

I can make a pull request for this but wanted feedback on the naming of the static path. I can imagine developer applications already having a /backoffice route - should we find another path instead of /backoffice/assets?

NduatiK commented 3 years ago

That's strange because I definitely fixed the CSS issue with 4f7a96d, perhaps I accidentally ran the command in a stale branch at some point..

No no. Your purging worked pretty well. I was just explaining why I was trying to squeeze out even more performance

Anyway, I'm not familiar with HTML catching but that does sound like it could worth pursuing! I only did it this way because that's what LiveDashboard did, haha

Caching happens automatically with Plug.Static

NduatiK commented 3 years ago

That's strange because I definitely fixed the CSS issue with 4f7a96d, perhaps I accidentally ran the command in a stale branch at some point..

Hmmm, 4f7a96d doesn't seem to belong to any branch.

edisonywh commented 3 years ago

I'm on Mobile so might've copied wrong link, this might work better: https://github.com/edisonywh/backoffice/pull/16

(EDIT: actually it's probably because I removed the branch after merging)

I had some slight reservation since this requires users to do a bit more work, but this is probably fine - if they don't do that, they'll just get an ugly page that doesn't work.

It would be great to see some screenshots of the Network tab showing the asset files are being cached if that's possible!

Also, do we need a Tuple for :from field? Shouldn't :backoffice work? (Same as what Kaffy does)

Same with :at, no need to put it in a /assets folder since it's already inside priv/static/backoffice

NduatiK commented 3 years ago

Screenshots:

With caching disabled: image

With caching enabled (the default): image

NduatiK commented 3 years ago

Also, do we need a Tuple for :from field? Shouldn't :backoffice work? (Same as what Kaffy does)

Same with :at, no need to put it in a /assets folder since it's already inside priv/static/backoffice

The /backoffice/assets just scoped the url. I was still serving files from backoffice's priv/static folder. That is, "/backoffice/assets/js/app.js" resolved to "/priv/static/js/app.js"

We could do the following instead

plug Plug.Static,
    at: "/backoffice",
    from: :backoffice,
    gzip: false,
    only: ~w(css js)

and in backoffice.html.eex:

    <link rel="stylesheet" href="/backoffice/css/app.css"/>
    <script src="/backoffice/js/app.js"></script>
NduatiK commented 3 years ago

What do you think about the backoffice url? I am worried that it might be a common application name in people's applications? For example, the Acme bank code has a backoffice application in its umbrella project

I would like to avoid stepping into other people's namespaces. What about at: "backoffice_lib" or at: "_backoffice"?

edisonywh commented 3 years ago

I think that looks great, but good point about the conflicting namespace..

The problem, as far as I understand, is with at: "/backoffice" right? And then in our backoffice.html.eex we assume it's there.

I am inclined to keep it this way, but we can circumvent this by allowing users to change their :at to any path, and then allow them to supply us the static_path via a callback in the Layout module, what do you think?

NduatiK commented 3 years ago

This would work. We can take this approach for now. I am not all that crazy about exposing these aspects of the implementation to users.

What do you think about expecting user's to use Backoffice.Layout when defining their layout file (instead of @behaviour Backoffice.Layout). Backoffice.Layout could be a macro that makes MyAppWeb.Backoffice.Layout into a plug with an overridable static_path function. Then users can add the plug to their endpoint.ex.

Something like this

# in /my_app_web/backoffice/layout.ex
defmodule MyAppWeb.Backoffice.Layout do

  # Defaults to "/backoffice" if not overriden
  def static_path(), do: "my_custom_route"  

  # ...  the stylesheets, scripts, logo and links function
end 

# in endpoint.ex
defmodule MyAppWeb.Endpoint do
   # ... other plugs

    plug(Plug.Static,
      at: "/",
      from: :my_app_web,
      gzip: false,
      only: ~w(css fonts images js favicon.ico robots.txt)
    )

  plug MyAppWeb.Backoffice.Layout
   # ... other plugs
end 
edisonywh commented 3 years ago

I'm not quite sure what the advantage is for using use, @behaviour works just fine for this use case no?

NduatiK commented 3 years ago

@behaviour is probably fine. I was just trying avoid to having to provide the same string in two places (when the backoffice path is not enough). We can stick to @behaviour.

I will update the pull request

edisonywh commented 3 years ago

Fixed in #52