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

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

Merged
merged 2 commits into from
Mar 10, 2025

Conversation

marcalexiei
Copy link
Contributor

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

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

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 added a commit to marcalexiei/eslint-doc-generator that referenced this pull request Mar 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
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