abevoelker / simple_form_tailwind_css

Tailwind components for Simple Form
MIT License
30 stars 3 forks source link

PurgeCSS with SimpleForm missing CSS Class #4

Closed jeremybdk closed 2 years ago

jeremybdk commented 2 years ago

Hello, I'm currently using your gem to replace our default simple form implementation. Everything was working fine locally but as soon as I started to deploy with PurgeCSS, the generated CSS was missing all the simple form classes.

I've added './config/initializers/simple_form.rb' to the list of content location to parse. My form is now working as expected.

However I'm running into issues with Heroicon, and Errors, I've started manually adding these classes to PurgeCSS safelist and I've manually added all these classes:

safelist: ['h-5', 'w-5', ,'h-5', 'w-5', 'text-red-500', 'absolute', 'inset-y-0', 'right-0', 'pr-3', 'items-center', 'pointer-events-none', 'mt-1', 'relative', 'rounded-md', 'shadow-sm']

And it's now working correctly, but I was wondering if there was a better way to do so?

Maybe adding this to the readme would be nice so that people knows that they should make sure that those classes are not purged.

Best,

abevoelker commented 2 years ago

Thank you for your report. I haven't used PurgeCSS but that definitely sounds like a not ideal scenario with having to maintain a safelist. Although I'm not sure if there's another alternative - I'm not super experienced with the interop between webpack et al and traditional Rails views.

If you wanted to open a PR to add that snippet to the README that would be greatly appreciated, and if you ever find a better way to do it I'd definitely like to support it.

abevoelker commented 2 years ago

After a quick glance through the Tailwind docs section addressing PurgeCSS I don't think there will be any alternative to the safelist approach. From the docs:

PurgeCSS (the library we use under the hood) is intentionally very naive in the way it looks for classes in your HTML. It doesn’t try to parse your HTML and look for class attributes or dynamically execute your JavaScript — it simply looks for any strings in the entire file that match this regular expression

Since it's just doing simple scanning of flat files I don't see how it could be intelligent enough to anticipate classes coming from a Ruby library dependency that's dynamically assembling the classes.

Therefore if you feel comfortable, I think adding your fixes to the bottom of the Installation section, below the rails g simple_form:tailwind:install would be the best remedy. In theory the generator could be modified to do this automatically but it's probably too complicated and error-prone to bother trying to make it automatic.

xanderificnl commented 2 years ago

Tailwind uses regular expressions to sift through files and find all classes that match its patterns. This means you can have it search through ruby files! Specifically, point it at ./config/initializers/simple_form.rb!

Tailwind 2's purge was renamed to content in v3. I'm using v3, so here's an excerpt from my tailwind.config.js:

  content: [
    './app/helpers/**/*.rb',
    './app/javascript/**/*.js',
    './app/views/**/*',
    './config/initializers/simple_form.rb'
  ],

It works without any issue, and I've verified (some grep 'n sed magic.) that all classes mentioned in simple_form.rb are included in a clean setup. There is no need to include a safe list, and the documentation actively discourages it.

-- If classes are hardcoded somewhere, it's probably best to refactor them to ./config/initializers/simple_form.rb

Do feel free to ping me if you need any help.

jeremybdk commented 2 years ago

@xanderificnl some classes are hardcoded in the files contained in simple_form_tailwind_css/lib/simple_form/tailwind/inputs/ that why they are currently needed in the safelist.

abevoelker commented 2 years ago

Thanks both for your reports and extra information. I agree with @xanderificnl the best path forward is to move all hardcoded CSS classes used in the input classes into constants defined in the initializer. Then add a note to the README to update tailwind.config.js to include the initializer in the content section. It will make the Ruby code harder to read but I guess that's what we need to do to appease PurgeCSS.

philliplongman commented 2 years ago

I'm also running into this issue using this with a Rails 7 app using importmap-rails and tailwindcss-rails.

Additionally, I'm not sure the dynamically generated classes you have are kosher. https://tailwindcss.com/docs/content-configuration#dynamic-class-names

For the time-being I've safelisted the following classes from the gem files:

  safelist: [
    "absolute",
    "bg-gray-50",
    "bg-red-50",
    "border",
    "border-gray-300",
    "border-l-0",
    "border-l-4",
    "border-r-0",
    "border-red-400",
    "flex",
    "flex-shrink-0",
    "h-5",
    "inline-flex",
    "inset-y-0",
    "items-center",
    "ml-3",
    "mt-1",
    "p-4",
    "pointer-events-none",
    "pr-3",
    "px-3",
    "relative",
    "right-0",
    "rounded-l-md",
    "rounded-md",
    "rounded-r-md",
    "shadow-sm",
    "sm:text-sm",
    "text-gray-500",
    "text-red-500",
    "text-red-700",
    "text-sm",
    "w-5"
  ]
abevoelker commented 2 years ago

Additionally, I'm not sure the dynamically generated classes you have are kosher. tailwindcss.com/docs/content-configuration#dynamic-class-names

Yeah you're right that definitely has to get changed to remove the string interpolation.

Hmm the more I think about it it would be nice to simply point the Tailwind config at the gem's full set of installed .rb files rather than manually generating a set of whitelisted classes that get copied to the initializer (which will get out of sync if classes change in later gem versions).

If Tailwind accepted an async function for content then I think something like this could work in tailwind.config.js:

const util = require('node:util')
const exec = util.promisify(require('node:child_process').exec)

/** @type {import('tailwindcss').Config} */
module.exports = {
  content: async () => {
    const { stdout } = await exec('bundle show simple_form_tailwind_css')
    return [
      `${stdout}/**/*.rb`,
      './app/views/**/*.{html,erb,haml}',
      './app/helpers/**/*.rb',
      './app/assets/stylesheets/**/*.{css,scss}',
      './app/javascript/**/*.{js,jsx,ts,tsx,vue}',
      './public/**/*.html',
    ]
  },
//...

Unfortunately it doesn't seem to work (Tailwind seems to just want content to be a flat list), and I'm not a good enough JavaScript developer to know off-hand how to make this work / if this is possible... if anyone has any ideas (or PRs) let me know

abevoelker commented 2 years ago

Call me crazy but for me, using Propshaft + the Tailwind CLI for builds, just passing the directory as an environment variable seems to work and may be the simplest solution:

  // package.json
  "scripts": {
    "build-dev:tailwind": "SIMPLE_FORM_TAILWIND_GEMDIR=\"`bundle show simple_form_tailwind_css`\" tailwindcss -i ./app/assets/stylesheets/application.tailwind.css -o ./app/assets/builds/tailwind.css --minify"
  }
}
// tailwind.config.js
/** @type {import('tailwindcss').Config} */
module.exports = {
  content: [
    `${process.env.SIMPLE_FORM_TAILWIND_GEMDIR}/**/*.rb`,
    './app/views/**/*.{html,erb,haml}',
    './app/helpers/**/*.rb',
    './app/assets/stylesheets/**/*.{css,scss}',
    './app/javascript/**/*.{js,jsx,ts,tsx,vue}',
    './public/**/*.html',
  ],
# Procfile.dev
tailwind: npm run build-dev:tailwind -- --watch

Thoughts? Would something like this work for your build setups?

philliplongman commented 2 years ago

We're still using Sprockets, because it seemed like Propshaft is still a little experimental. Because we're using importmap-rails for the JS, there's no package.json.

Perhaps it might be valuable for you to raise this issue in the tailwind-rails repo (this one looks like it might be related)? You can't be the only developer who's going to include templates and html-building logic inside a gem.

abevoelker commented 2 years ago

@philliplongman okay, is your Procfile calling the tailwindcss CLI from somewhere?

abevoelker commented 2 years ago

@philliplongman it looks like that gem adds Rake tasks that get called by Foreman. Untested since I don't use the gem but something like this should be able to pass the environment variable to tailwind.config.js

# lib/tasks/tailwindcss.rake

TAILWIND_COMPILE_COMMAND = "#{RbConfig.ruby} #{Pathname.new(__dir__).to_s}/../../exe/tailwindcss -i '#{Rails.root.join("app/assets/stylesheets/application.tailwind.css")}' -o '#{Rails.root.join("app/assets/builds/tailwind.css")}' -c '#{Rails.root.join("config/tailwind.config.js")}' --minify"
SIMPLE_FORM_TAILWIND_GEMDIR = `bundle show simple_form_tailwind_css`

Rake::Task["tailwindcss:build"].clear
Rake::Task["tailwindcss:watch"].clear
namespace :tailwindcss do
  desc "Build your Tailwind CSS"
  task :build do
    system({"SIMPLE_FORM_TAILWIND_GEMDIR" => SIMPLE_FORM_TAILWIND_GEMDIR}, TAILWIND_COMPILE_COMMAND, exception: true)
  end

  desc "Watch and build your Tailwind CSS on file changes"
  task :watch do
    system({"SIMPLE_FORM_TAILWIND_GEMDIR" => SIMPLE_FORM_TAILWIND_GEMDIR}, "#{TAILWIND_COMPILE_COMMAND} -w")
  end
end
abevoelker commented 2 years ago

I've added instructions to the README on how to fix this issue using the env var approach, and removed the :color option from the f.error_notification helper.

So I'm going to presume this fixed as of v1.0.0 which I just pushed. Thanks everyone for your reports

philliplongman commented 2 years ago

Here is a tweaked version of the rake task, which does work in my setup. You don't need to set TAILWIND_COMPILE_COMMAND because it's still defined from the tailwindcss-rails version of the task.

# lib/tasks/tailwindcss.rake

Rake::Task["tailwindcss:build"].clear
Rake::Task["tailwindcss:watch"].clear

namespace :tailwindcss do
  desc "Build your Tailwind CSS"
  task build: :environment do
    system "#{env_variable} TAILWIND_COMPILE_COMMAND", exception: true
  end

  desc "Watch and build your Tailwind CSS on file changes"
  task watch: :environment do
    system "#{env_variable} #{TAILWIND_COMPILE_COMMAND} -w"
  end

  def env_variable
    "SIMPLE_FORM_TAILWIND_CSS=#{Gem.loaded_specs['simple_form_tailwind_css'].gem_dir}"
  end
end
abevoelker commented 2 years ago

Perfect could you make a PR to fix the README code snippet

On Wed, Aug 31, 2022 at 6:33 PM Phillip Longman @.***> wrote:

Here is a tweaked version of the rake task, which does work in my setup. You don't need to set TAILWIND_COMPILE_COMMAND because it's still defined from the tailwindcss-rails version of the task.

lib/tasks/tailwindcss.rake

Rake::Task["tailwindcss:build"].clearRake::Task["tailwindcss:watch"].clear namespace :tailwindcss do desc "Build your Tailwind CSS" task build: :environment do system "#{env_variable} TAILWIND_COMPILE_COMMAND", exception: true end

desc "Watch and build your Tailwind CSS on file changes" task watch: :environment do system "#{env_variable} #{TAILWIND_COMPILE_COMMAND} -w" end

def env_variable "SIMPLE_FORM_TAILWIND_CSS=#{Gem.loaded_specs['simple_form_tailwind_css'].gem_dir}" endend

— Reply to this email directly, view it on GitHub https://github.com/abevoelker/simple_form_tailwind_css/issues/4#issuecomment-1233552825, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFO437BMPTNTE7M3SJZZ3V37TVBANCNFSM5IBUC42Q . You are receiving this because you modified the open/close state.Message ID: @.***>