8000 Convert plugin.js to ESM because package.json has type set to module by 4ndv · Pull Request #8536 · activeadmin/activeadmin · GitHub
[go: up one dir, main page]
Skip to content

Convert plugin.js to ESM because package.json has type set to module #8536

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 1 commit into from
Nov 21, 2024
Merged

Convert plugin.js to ESM because package.json has type set to module #8536

merged 1 commit into from
Nov 21, 2024

Conversation

4ndv
Copy link
Contributor
@4ndv 4ndv commented Nov 6, 2024

This PR makes plugin.js use ESM syntax instead of CJS, because package.json has "type": "module" which causes issues when using bundlers.

I've tested this change against activeadmin demo repo, replacing the file in node_modules, and it works fine with the oldest non-EOL version of node, 18.20.4.

Looks like autogenerated tailwind config can require() ESM version just fine, so probably there is no need to ship both CJS and ESM versions.

Fixes #8534

4ndv added a commit to 4ndv/activeadmin that referenced this pull request Nov 6, 2024
@4ndv
Convert plugin.js to ESM because package.json has type set to module (a…
e9c25dd 
…ctiveadmin#8536)
@4ndv 4ndv force-pushed the convert-plugin-to-esm branch from 8b10b04 to e9c25dd Compare November 6, 2024 18:10
@4ndv
Copy link
Contributor Author
4ndv commented Nov 6, 2024 •
edited
Loading

Well, I've tried to test these changes with Vite and looks like I've completely borked my previous attempt, because it works without these changes, and also there is no JIT issue I've mentioned previously.

Not sure if this PR should be closed or not, I don't think this change will do any harm, but the original issue looks like a problem on my side.

Btw here's my repo with aa4+vite example, because my previous attempt was on work machine, and it's a PITA to separate from the project, so I've started from the clean state

@4ndv
Copy link
Contributor Author
4ndv commented Nov 6, 2024

Reading the error message from issue again, I still think that this PR is valid, because the message states very clearly that all of the files inside npm package marked as ESM should be in ESM style and this mix and match can cause some issues.

@javierjulio
Copy link
Member

@4ndv thank you. We'll keep this open since it makes sense to use ESM here and to ensure compatibility with Vite. I will try to find time soon to test this locally where not using Vite. Let us know what you find out when you have time. Thanks again.

@codecov Codecov
Copy link
codecov bot commented Nov 7, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (a19afe4) to head (4ea6065).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #8536   +/-   ##
=======================================
  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.

🚨 Try these New Features:

@til
Copy link
Contributor
til commented Nov 11, 2024 •
edited
Loading

Maybe of interest: after upgrading node from version 20 to 23 in a project with AA 4.0.0.beta13, I saw an error that looks different than yours but (I think) has a related reason:

22:45:19 css.1  | (node:17401) ExperimentalWarning: CommonJS module /home/tils/basistool/tailwind-active_admin.config.js is loading ES Module /home/tils/basistool/node_modules/@activeadmin/activeadmin/plugin.js using require().
22:45:19 css.1  | Support for loading ES Module in require() is an experimental feature and might change at any time
22:45:19 css.1  | (Use `node --trace-warnings ...` to show where the warning was created)
22:45:19 css.1  | file:///home/tils/basistool/node_modules/@activeadmin/activeadmin/plugin.js:1
22:45:19 css.1  | const plugin = require('tailwindcss/plugin')
22:45:19 css.1  |                ^
22:45:19 css.1  | 
22:45:19 css.1  | ReferenceError: require is not defined
22:45:19 css.1  |     at file:///home/tils/basistool/node_modules/@activeadmin/activeadmin/plugin.js:1:16
22:45:19 css.1  |     at ModuleJobSync.runSync (node:internal/modules/esm/module_job:367:35)
22:45:19 css.1  |     at ModuleLoader.importSyncForRequire (node:internal/modules/esm/loader:325:47)
22:45:19 css.1  |     at loadESMFromCJS (node:internal/modules/cjs/loader:1396:24)
22:45:19 css.1  |     at Module._compile (node:internal/modules/cjs/loader:1529:5)
22:45:19 css.1  |     at Object..js (node:internal/modules/cjs/loader:1678:16)
22:45:19 css.1  |     at Module.load (node:internal/modules/cjs/loader:1315:32)
22:45:19 css.1  |     at Function._load (node:internal/modules/cjs/loader:1125:12)
22:45:19 css.1  |     at TracingChannel.traceSync (node:diagnostics_channel:322:14)
22:45:19 css.1  |     at wrapModuleLoad (node:internal/modules/cjs/loader:216:24)
22:45:19 css.1  |

Applying this PR fixed it.

@4ndv
Copy link
Contributor Author
4ndv commented Nov 12, 2024 •
edited
Loading

Maybe of interest: after upgrading node from version 20 to 23 in a project with AA 4.0.0.beta13, I saw an error that looks different than yours but (I think) has a related reason:

Just tested running node 23.1.0 with AA demo repo and can confirm this:

▲ demo.activeadmin.info [main] yarn build:css
yarn run v1.22.19
$ tailwindcss -i ./app/assets/stylesheets/active_admin.css -o ./app/assets/builds/active_admin.css --minify -c tailwind-active_admin.config.js

Rebuilding...
(node:497283) ExperimentalWarning: CommonJS module /home/lynx/projects/demo.activeadmin.info/tailwind-active_admin.config.js is loading ES Module /home/lynx/projects/demo.activeadmin.info/node_modules/@activeadmin/activeadmin/plugin.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
file:///home/lynx/projects/demo.activeadmin.info/node_modules/@activeadmin/activeadmin/plugin.js:1
const plugin = require('tailwindcss/plugin')
               ^

ReferenceError: require is not defined

I always prefer sticking with LTS versions of node ("even" ones, 20.x, 22.x) so haven't even tested 23, thank you!

Looks like the warning is caused because default tailwind-active_admin.config.js is in CJS trying to require AA plugin, which node thinks is ESM, and the exception is thrown because plugin.js processed by ESM loader, and in ESM context there is no global require.

@tagliala
Copy link
Contributor
tagliala commented Nov 15, 2024 •
edited
Loading

Confirmed issue with Node 23

#8534 (comment)

@javierjulio javierjulio self-assigned this Nov 20, 2024
@javierjulio javierjulio self-requested a review November 20, 2024 23:38
javierjulio
javierjulio approved these changes Nov 20, 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 pushed a commit to 4ndv/activeadmin that referenced this pull request Nov 20, 2024
@4ndv @javierjulio
Convert plugin.js to ESM because package.json has type set to module (a…
bdc38af 
…ctiveadmin#8536)
@javierjulio javierjulio force-pushed the convert-plugin-to-esm branch from e9c25dd to bdc38af Compare November 20, 2024 23:38
@tagliala
Copy link
Contributor

This deserves an entry in the changelog, because it also fixes compatibility with Node 23

@javierjulio
Copy link
Member

@tagliala yes, it will be part of the release notes since its generated automatically based on the PR label.

@javierjulio javierjulio force-pushed the convert-plugin-to-esm branch from bdc38af to 4ea6065 Compare November 21, 2024 00:08
@javierjulio javierjulio merged commit 94dd987 into activeadmin:master Nov 21, 2024
20 checks passed
@wrozka wrozka mentioned this pull request Dec 6, 2024
Merged
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

@javierjulio javierjulio

Labels
type bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v4: provide plugin.js in both mjs and cjs variants (Node 23)
4 participants
@4ndv @javierjulio @til @tagliala
0