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

Force skipping vite dev server #93

Closed
wants to merge 1 commit into from

Conversation

Intrepidd
Copy link

Hi ! Thanks for the great work with vite_ruby ⚡

I was looking into integrating it into my app but got blocked because of premailer

Premailer needs to read and parse a CSS file to be able to inline it properly. Obviously it doesn't work when the dev server is enabled as it expects a classic css file and not ESM.

This PR is a total draft / POC, it aims at being able to skip the vite server, and grab a compiled file when we want to, basically force the on demand behaviour even when the dev server is running.

The wording is very bad, force_build isn't correct and should be changed if this goes further than this draft.

I just wanted to get your opinion on this, do you see this as something that could make its way into the codebase (if re-done properly obviously), are there any gotchas I didn't see ?

Here is what I had to do :

  • Add an option to "force a build", which is rather a "force skip the dev server"
  • In the dev server proxy, don't forward to the dev server thanks to a URL param

Thanks !

def lookup(name, type: nil)
@build_mutex.synchronize { builder.build } if should_build?
def lookup(name, type: nil, force_build: false)
@build_mutex.synchronize { builder.build } if force_build || should_build?
Copy link
Author

Choose a reason for hiding this comment

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

This is wrong, we don't want to force build in production for example.

The correct way should be something like config.auto_build && (!dev_server_running? || force_build)

@ElMassimo
Copy link
Owner

Hi Adrien!

On my phone right now, I'll get back to this during the week.

At a first glance it seems that premailer supports fetching css from a URI, so perhaps it's a matter of configuring the resolution so that it doesn't assume it's a local file.

That would have the added benefit of being extremely fast during development, as Vite would only process the requested stylesheets instead of triggering a full build.

@Intrepidd
Copy link
Author

Intrepidd commented Jun 20, 2021 via email

@ElMassimo
Copy link
Owner

ElMassimo commented Jun 20, 2021

If you can share a minimal repo with the intended usage I can take a look.

Vite does not serve the file as-is—it would run it through sass and process any imports within the file, so premailer should be able to parse it correctly.

@Intrepidd
Copy link
Author

I understand what you mean.

I think the main issue is that the file premailer reads contains import statements. While the browser can resolve those premailer expects a complete single css file.

Is that clearer? Otherwise I will provide with a repro

@Intrepidd
Copy link
Author

Anyway here's a repro :) https://github.com/Intrepidd/vite_ruby_issue

Steps are described in the readme, let me know if I can help

@ElMassimo
Copy link
Owner

ElMassimo commented Jun 22, 2021

Thanks for sharing a sample repo. As I mentioned before, it's possible to request the processed CSS from Vite.

This is insanely faster than building, and it seems to work nicely in the example repo 😃

@Intrepidd Please see the following options, and let me know how it goes!


The default NetworkLoader strategy in premailer-rails would work out of the box (in your example, by setting config.asset_host = 'http://localhost:1111').

The problem is that no Accept header is sent in the request, and Vite will serve JS by default.

I opened a PR in premailer-rails to add Accept: text/css in the request by default.


In the meantime, you could define a custom strategy for Premailer, for example, in config/initializers/premailer.rb:

if Rails.env.development?
  module ViteNetworkLoader
    def self.load(url)
      return unless ViteRuby.instance.dev_server_running?
      return unless url.start_with?("/#{ViteRuby.config.public_output_dir}/")
      Net::HTTP.get(URI("http://localhost:1111#{url}"), { 'Accept' => 'text/css' }).presence
    end
  end

  Premailer::Rails.config.fetch(:strategies) << ViteNetworkLoader
end
Patch for your example app
diff --git a/app/views/layouts/mailer.html.erb b/app/views/layouts/mailer.html.erb
index 76ce8b3..d654ce3 100644
--- a/app/views/layouts/mailer.html.erb
+++ b/app/views/layouts/mailer.html.erb
@@ -2,7 +2,7 @@
 <html>
   <head>
     <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
-    <%= vite_stylesheet_tag 'application.scss', host: "http://localhost:1111" %>
+    <%= vite_stylesheet_tag 'application.scss' %>
   </head>
 
   <body>
diff --git a/config/initializers/premailer.rb b/config/initializers/premailer.rb
new file mode 100644
index 0000000..18c51c2
--- /dev/null
+++ b/config/initializers/premailer.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+if Rails.env.development?
+  module ViteNetworkLoader
+    options = Rails.application.config.action_mailer.default_url_options
+    HOST_WITH_PORT = "http://#{options.fetch(:host)}:#{options.fetch(:port)}"
+
+    def self.load(url)
+      return unless ViteRuby.instance.dev_server_running?
+      return unless url.start_with?("/#{ViteRuby.config.public_output_dir}/")
+      Net::HTTP.get(URI("#{HOST_WITH_PORT}#{url}"), { 'Accept' => 'text/css' }).presence
+    end
+  end
+
+  Premailer::Rails.config.fetch(:strategies) << ViteNetworkLoader
+end

@ElMassimo ElMassimo closed this Jun 22, 2021
ElMassimo added a commit to ElMassimo/premailer-rails that referenced this pull request Jun 22, 2021
This allows development servers to serve the correct file type.

For more information see ElMassimo/vite_ruby#93 (comment)
@Intrepidd
Copy link
Author

Thanks!

I had no idea you could do that with the accept header! I was confused because from my browser or curl I would see the import statements.

Sorry for the confusion and thanks for your help!

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