Skip to content

Use ESM syntax in tailwind config template #8568

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 7, 2024
Merged

Use ESM syntax in tailwind config template #8568

merged 3 commits into from
Dec 7, 2024

Conversation

wrozka
Copy link
Contributor

@wrozka wrozka commented Dec 6, 2024 •
edited by javierjulio
Loading

A recent syntax change in plugin.js from #8536 causes tailwind watch to break. It runs successfully on first run, but then it builds only partially - it doesn't crash, but the output files don't contain all the CSS they should.

I'm not entirely sure what causes it to break only on reloads, but the current version on tailwind config produces this warning:

ExperimentalWarning: CommonJS module /Users/wrozka/Projects/orders/tailwind-active_admin.config.js is loading ES Module /Users/wrozka/Projects/orders/node_modules/@activeadmin/activeadmin/plugin.js using require().

This PR fixes the broken tailwind watch by migrating tailwind config from CJS to ESM, but it doesn't resolve the warning, it just moves it one level up the stack trace:

ExperimentalWarning: CommonJS module /Users/wrozka/Projects/orders/node_modules/tailwindcss/lib/lib/load-config.js is loading ES Module /Users/wrozka/Projects/orders/tailwind-active_admin.config.js using require().

I don't think this is the final fix, but at least it fixes the watch problem.

@javierjulio javierjulio changed the title Use ESM syntax in tailwind template Use ESM syntax in tailwind config template Dec 6, 2024
javierjulio
javierjulio reviewed Dec 6, 2024
View reviewed changes
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

@wrozka thank you. As you mentioned the warning will still occur. I don't know why. When reviewing tailwindcss-forms, the documentation still uses the CJS version. I would think both CJS and ESM work though. The warning could be coming from the ActiveAdmin side.

Your change does resolve the watch issue, we just need to update the template file used by our dev/test app generator. With the requested changes, the test suite should go green and the local dev app should work now.

Please open the following file and edit this single line:

activeadmin/spec/support/rails_template.rb

Line 100 in 5f4ee6c

gsub_file "tailwind-active_admin.config.js", /@activeadmin\/activeadmin/, "${activeAdminPath}"

to the following so it will do a local require for development:

gsub_file "tailwind-active_admin.config.js", Regexp.new("@activeadmin/activeadmin/plugin"), "../../../plugin"

@codecov Codecov
Copy link

codecov bot commented Dec 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (5f4ee6c) to head (bfa3c51).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #8568   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4076     4076           
=======================================
  Hits         4040     4040           
  Misses         36       36

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

@wrozka wrozka requested a review from javierjulio December 6, 2024 20:22
javierjulio
javierjulio approved these changes Dec 7, 2024
View reviewed changes
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thanks!

@javierjulio javierjulio merged commit 9c27c84 into activeadmin:master Dec 7, 2024
22 checks passed
thibaudgg added a commit to csa-admin-org/csa-admin that referenced this pull request Dec 8, 2024
@thibaudgg
ActiveAdmin beta and rails_icons 1.0 updates
889cbb5 
Fix the tailwindcss:watch bug:
activeadmin/activeadmin#8568
@matteeyah
Copy link

This breaks for Rails installations that don't have a node runtime with @activeadmin/activeadmin available in development or during deployment. This is most new Rails installs, since they use importmaps by default, instead of the legacy way of including a package.json and a full node environment.

#19 10.94 Error: Cannot find module '@activeadmin/activeadmin/plugin'
#19 10.94 Require stack:
#19 10.94 - /rails/config/tailwind-active_admin.config.js
#19 10.94     at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
#19 10.94     at Function._resolveFilename (pkg/prelude/bootstrap.js:1951:46)
#19 10.94     at Function.resolve (node:internal/modules/cjs/helpers:108:19)
#19 10.94     at Function.resolve (/snapshot/tailwindcss/standalone-cli/patch-require.js:34:38)
#19 10.94     at _resolve (/snapshot/tailwindcss/node_modules/jiti/dist/jiti.js:1:241814)
#19 10.94     at jiti (/snapshot/tailwindcss/node_modules/jiti/dist/jiti.js:1:244531)
#19 10.94     at /rails/config/tailwind-active_admin.config.js:2:15
#19 10.94     at evalModule (/snapshot/tailwindcss/node_modules/jiti/dist/jiti.js:1:247313)
#19 10.94     at jiti (/snapshot/tailwindcss/node_modules/jiti/dist/jiti.js:1:245241)
#19 10.94     at /snapshot/tailwindcss/lib/lib/load-config.js:52:26 {
#19 10.94   code: 'MODULE_NOT_FOUND',
#19 10.94   requireStack: [ '/rails/config/tailwind-active_admin.config.js' ]
#19 10.94 }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierjulio javierjulio javierjulio approved these changes

Assignees
No one assigned
Labels
type template update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wrozka @matteeyah @javierjulio