octokit / plugin-rest-endpoint-methods.js

Octokit plugin adding one method for all of api.github.com REST API endpoints
MIT License
116 stars 56 forks source link

[BUG]: CommonJS `require()` is broken. #744

Closed manchicken closed 5 months ago

manchicken commented 5 months ago

What happened?

I just upgraded, and it looks like the updated version (13.2.1) breaks compatibility with CommonJS require().

This line of code:

const { restEndpointMethods } = require('@octokit/plugin-rest-endpoint-methods')

Results in this exception:

Error [ERR_REQUIRE_ESM]: require() of ES Module

Typically modules like this would have a version which is suitable for both ESM and CJS, and this used to have that (in 10.2.0).

Versions

I'm on Node v22.2.0, and here's the diff in the package-lock.json:

-        "@octokit/plugin-paginate-rest": "^9.1.5",
-        "@octokit/plugin-rest-endpoint-methods": "^10.2.0",
-        "@octokit/plugin-throttling": "^8.1.3",
-        "@octokit/rest": "^20.0.2",
+        "@octokit/plugin-paginate-rest": "^11.3.0",
+        "@octokit/plugin-rest-endpoint-methods": "^13.2.1",
+        "@octokit/plugin-throttling": "^9.3.0",
+        "@octokit/rest": "^20.1.1",

Relevant log output

No response

Code of Conduct

github-actions[bot] commented 5 months ago

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

wolfy1339 commented 5 months ago

This is expected. This package has migrated to only ESM.

gr2m commented 5 months ago
-        "@octokit/plugin-paginate-rest": "^9.1.5",
-        "@octokit/plugin-rest-endpoint-methods": "^10.2.0",
-        "@octokit/plugin-throttling": "^8.1.3",
-        "@octokit/rest": "^20.0.2",
+        "@octokit/plugin-paginate-rest": "^11.3.0",
+        "@octokit/plugin-rest-endpoint-methods": "^13.2.1",
+        "@octokit/plugin-throttling": "^9.3.0",
+        "@octokit/rest": "^20.1.1",

you shouldn't need to import the paginate and rest endpoint methods plugins, they are all included in @octokit/rest: https://github.com/octokit/rest.js/blob/main/src/index.ts

Just update @octokit/plugin-throttling, but stay below v9 which migrated to ESM

manchicken commented 5 months ago

This is expected. This package has migrated to only ESM.

That's disappointing. This breaks a bunch of stuff.

wolfy1339 commented 5 months ago

Every major version contains breaking changes. Updating without reading the release notes is bad

Unfortunately, this is the way the ecosystem is going, and has been a direction we were planning to pursue for a while

manchicken commented 5 months ago

you shouldn't need to import the paginate and rest endpoint methods plugins, they are all included in @octokit/rest: https://github.com/octokit/rest.js/blob/main/src/index.ts

I'll look into that.

Just update @octokit/plugin-throttling, but stay below v9 which migrated to ESM

I don't think they've so much migrated to ESM so much as they've just made the choice to require their users to do so.

gr2m commented 5 months ago

We have to migrate to ESM as the ecosystem is migrating to ESM. You should upgrade as well in order to avoid to get stuck with dependencies that have known security problems.

Your last comment confusing and unnecessarily confrontational. I understand the ESM transition is causing friction like it does for everyone.

If you are stuck with CommonJS for a while, that's ok. We will backport security fixes to the older versions, and you are free to submit pull requests against these older versions.

manchicken commented 5 months ago

We have to migrate to ESM as the ecosystem is migrating to ESM. You should upgrade as well in order to avoid to get stuck with dependencies that have known security problems.

Why do y'all keep saying that the ecosystem is migrating to ESM when most major modules still support CJS, and all major runtimes still maintain support for CJS? While I understand some folks have a preference for one over the other, I haven't seen anybody adopting the stance that this project now is.