Skip to content

Fix ESM support in Node 22+ when the package includes the main property #648

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
bmish merged 2 commits into from
Mar 10, 2025
Merged

Fix ESM support in Node 22+ when the package includes the main property #648

bmish merged 2 commits into from
Mar 10, 2025

Conversation

@marcalexiei
Copy link
Contributor

@marcalexiei marcalexiei commented Mar 10, 2025

Try to close #615

This PR adds a check in the loadPlugin require inside package-json.ts to verify if the returned value is an ESModule, in an attempt to resolve #615.

Warning

I was unable to create a fixture inside test/fixtures to add a test to avoid regression.
I created a new fixture named esm with a package.json of type module, including main and exports entries.
However, when running jest, the require call consistently threw an error, making my code unreachable.

In test/lib/generate/cjs-test.ts,
within the 'package.json main field points to a non-existent file' test,
I updated the FIXTURE_PATH generation to align with the pattern used in other tests.

@marcalexiei marcalexiei marked this pull request as ready for review March 10, 2025 06:46
@bmish
Copy link
Owner

bmish commented Mar 10, 2025

Thank you for the PR.

Have you done some manual testing to verify if this improves the behavior and avoids regressions?

I understand some of these scenarios can be difficult to test with our testing setup. You may need to add a istanbul ignore comment for untestable lines to pass CI.

@bmish bmish added the bug Something isn't working label Mar 10, 2025
@marcalexiei
Copy link
Contributor Author

marcalexiei commented Mar 10, 2025

Have you done some manual testing to verify if this improves the behaviour and avoids regressions?

Yes, I tested the patch in the repository provided in the issue reproduction.
I applied the patch directly on node_modules/eslint-doc-generator/dist/lib/package-json.js.
I tested using the following node versions:

  • v20.18.2
  • v22.14.0

I understand some of these scenarios can be difficult to test with our testing setup. You may need to add an istanbul ignore comment for untestable lines to pass CI.

Done, now it should pass.

@bmish
Copy link
Owner

bmish commented Mar 10, 2025

Thanks. What would be a good user-facing title for this fix? E.g. "Fix ESM support in Node 22+"

@marcalexiei
Copy link
Contributor Author

marcalexiei commented Mar 10, 2025

This bug occurs only when the main property is set, so I propose:

Fix ESM support in Node 22+ when the package includes the `main` property

@bmish bmish changed the title fix(package-json): loadPlugin - check if require returns a ESM module Fix ESM support in Node 22+ when the package includes the main property Mar 10, 2025
@bmish bmish merged commit 82bf823 into bmish:main Mar 10, 2025
8 checks passed
@bmish
Copy link
Owner

bmish commented Mar 10, 2025

Thanks!

@marcalexiei marcalexiei deleted the require-node22-returns-esm branch March 11, 2025 05:51
@marcalexiei marcalexiei mentioned this pull request Mar 11, 2025
Merged
@bmish bmish mentioned this pull request Mar 15, 2025
Closed
marcalexiei added a commit to marcalexiei/eslint-doc-generator that referenced this pull request Mar 15, 2025
@marcalexiei
Revert "Fix ESM support in Node 22+ when the package includes the `ma…
4e58192 
…in` property (bmish#648)"

This reverts commit 82bf823.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bmish bmish bmish approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: Node 22 - "Could not find exported rules object in ESLint plugin." error

2 participants

@marcalexiei @bmish