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

Include partials in NPM package for Tailwind config #8220

Closed
wants to merge 1 commit into from

Conversation

hasghari
Copy link
Contributor

@hasghari hasghari commented Jan 7, 2024

The current tailwind configuration depends on the presence of the ruby gem. This is not flexible for all build systems and processes. By publishing all the files referenced in the tailwind config to npm, we can remove this dependency so that a yarn or npm build can run independently.

Copy link

codecov bot commented Jan 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (af7dcdc) 99.10% compared to head (fe9fb8a) 99.10%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8220   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files         140      140           
  Lines        4018     4018           
=======================================
  Hits         3982     3982           
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hasghari
Copy link
Contributor Author

hasghari commented Jan 7, 2024

Right now in order to work around this issue in my setup, I have copied all the views by running rails g active_admin:views and updated the tailwind config to the following:

module.exports = {
  content: [
    './node_modules/flowbite/dist/flowbite.js',
    './node_modules/@activeadmin/activeadmin/plugin.js',
    './app/admin/**/*.{arb,erb,html,rb}',
    './app/views/active_admin/**/*.{arb,erb,html,rb}',
    './app/views/layouts/active_admin.html.erb',
    './app/views/layouts/active_admin_logged_out.html.erb',
    './app/views/admin/**/*.{arb,erb,html,rb}',
    './app/javascript/**/*.js'
  ],
  darkMode: 'class',
  plugins: [require(`@activeadmin/activeadmin/plugin`)]
}

Once all the required files have been published to npm, I could change the config to the following and remove all the files copied with the rake task that I don't intend on customizing:

const activeAdminPath = './node_modules/@activeadmin/activeadmin'

module.exports = {
  content: [
    `${activeAdminPath}/vendor/javascript/flowbite.js`,
    `${activeAdminPath}/plugin.js`,
    `${activeAdminPath}/app/views/**/*.{arb,erb,html,rb}`,
    './app/admin/**/*.{arb,erb,html,rb}',
    './app/views/active_admin/**/*.{arb,erb,html,rb}',
    './app/views/admin/**/*.{arb,erb,html,rb}',
    './app/javascript/**/*.js'
  ],
  darkMode: "class",
  plugins: [
    require(`@activeadmin/activeadmin/plugin`)
  ]
}

@javierjulio
Copy link
Member

Hi @hasghari, thank you. I would need to know why this is an issue in some build steps with more detail. Can you please provide info on what that system looks like and why it wouldn't have access to the Ruby gem? Would be worth including all the vendor JS files in the NPM package at least so we could do that separately while hashing out the rest. I just don't see/understand the need for the other changes. Thanks!

package.json Outdated Show resolved Hide resolved
@hasghari
Copy link
Contributor Author

hasghari commented Jan 7, 2024

@javierjulio My current setup deploys to Heroku using the node buildpack first and then the ruby buildpack. When the node buildpack runs, it runs yarn build where all the CSS and JS bundling happens. Since the ruby buildpack hasn't run yet, this means the activeadmin gem isn't available which leads to the build failing.

One workaround for this that I found was using the cssbundling-rails gem which will run right before rails assets:precompile. However, this means another redundant yarn install and yarn build:css has to run but more importantly, there's no easy way to prune the dev dependencies and they unnecessarily bloat the final Heroku slug.

If I had all the required files available in the npm package, then I would have no dependency on the gem for running yarn build:css and also no need to use cssbundling-rails gem.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@javierjulio javierjulio changed the title Publish all files referenced in tailwind config Include partials in NPM package for Tailwind config Jan 8, 2024
@javierjulio
Copy link
Member

For reference, although it impacts jsbundling-rails:

Eventually, that last change also made it's way into cssbundling-rails via rails/cssbundling-rails#125 which should address your issue here since the approach is for the standard asset pipeline.

@hasghari
Copy link
Contributor Author

hasghari commented Jan 8, 2024

Unfortunately the environment variable to skip yarn install doesn't address the issue since all of the packages I need for bundling CSS and JS are listed under devDependencies and they get pruned before the heroku/node buildpack completes execution. Therefore, the cssbundling-rails task needs to run yarn install again in order to fetch those devDependencies and run the yarn build:css script.

@jeremybdk
Copy link

I've also tried to test this beta version with our current setup (shakapacker + tailwind).
Currently it won't work as ActiveAdmin require importmap-rails but this could be a step in the right direction for us, as being able to customise our setup to have our current tailwind config parse the ActiveAdmin files to generate the correct CSS files.

The current tailwind configuration depends on the presence of the ruby gem.
This is not flexible for all build systems and processes. By publishing all
the files referenced in the tailwind config to npm, we can remove this
dependency so that a yarn or npm build can run independently.
@javierjulio
Copy link
Member

@hasghari only conclusion I come to here is that the approach is flawed. I don't see why this would be necessary since in your case you need to be using cssbundling-rails as you need to precompile assets and serve them. This comes across as a workaround for Heroku that is specific to your setup but not what others on Heroku would need or want if using Node and Rails. I really don't think it's the answer for us to be putting the Ruby files in the NPM package to accommodate a workaround or optimization, whereas that seems intentional to me with Heroku. Sorry.

@hasghari
Copy link
Contributor Author

@javierjulio I agree that including the view files feels odd but in my opinion it's a less flawed solution than the Tailwind configuration expecting the presence of the activeadmin gem.

I actually don't need to be using cssbundling-rails and don't use it in my application because I've intentionally kept my node and ruby build processes separate. I don't think this is a Heroku-specific change and that other people with more involved build systems will benefit from this change. Nevertheless, I accept your decision to reject this change.

@javierjulio
Copy link
Member

@hasghari thank you.

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.

None yet

4 participants