feat: esm only version #1825

pixelass · 2023-05-26T18:42:23Z

This Pull Request attpempts to make probot esm compatible.

The current implementation creates a BREAKING CHANGE as it migrates probot to esm-only

The process is as follows

Local prototype to measure the required work
Make sure all tests pass
Propose esm-only as new major

Local test output

💡 It seems that only the test setup needs some tweaking.
The app seems to work as intended.

Test Suites: 6 failed, 10 passed, 16 total
Tests:       10 failed, 2 skipped, 108 passed, 120 total
Snapshots:   1 failed, 5 passed, 6 total
Time:        7.056 s
Ran all test suites.

closes: #1684

switch update-dotenv back to npm package

sort imports run prettier

update jest to latest run esm compatible tests

welcome · 2023-05-26T18:42:25Z

Thanks for opening this pull request! A contributor should be by to give feedback soon. In the meantime, please check out the contributing guidelines and explore other ways you can get involved.

request-info · 2023-05-26T18:42:43Z

Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details?

make previously removed version check esm compatible

Thanks to: probot@0d74f5f

travi · 2023-06-23T19:02:28Z

💡 It seems that only the test setup needs some tweaking.
The app seems to work as intended.

i'm curious if the test complications are simply related to jest having trouble within an esm context. related: jestjs/jest#9430

i've had a good experience moving projects from jest to vitest when converting to esm and it is api compatible with jest, so the test changes should be minimal. would a switch between test frameworks be a change that you'd be open to try as part of this effort, @pixelass?

travi

thanks for your efforts with this @pixelass. i added a few minor comments based on a first-pass review, but looks like this is headed in a great direction.

travi · 2023-06-23T19:03:33Z

bin/probot-receive.js

@@ -1,3 +1,3 @@
 #!/usr/bin/env node

-require("../lib/bin/probot-receive");
+import "../lib/bin/probot-recieve.js";


Suggested change

import "../lib/bin/probot-recieve.js";

import "../lib/bin/probot-receive.js";

i assume this change was accidental since the referenced file is still spelled the previous way, without the typo

travi · 2023-06-23T19:57:21Z

src/apps/default.ts

-      pkg = require(path.join(process.cwd(), "package.json"));
+      pkg = (
+        await import(path.join(process.cwd(), "package.json"), {
+          assert: { type: "json" },


import assertions are still considered experimental, even in v20, so i'm curious if we need a different approach with this. does typescript happen to handle this differently somehow?

travi · 2023-06-23T19:59:03Z

src/bin/probot.ts


-const pkg = require("../../package");
+const pkg = JSON.parse(
+  fs.readFileSync(path.resolve(__dirname, "../../package.json"), {


this might be the safer option for loading the package file, but wherever we land, it would be best to be consistent

travi · 2023-06-23T20:05:47Z

test/probot.test.ts

-A1yBjz3q2nX+zthk+GLXrJQkYOnIk1ECIHfeFV8TWm5gej1LxZquBTA5pINoqDVq
-NKZSuZEHqGEFAiB6EDrxkovq8SYGhIQsJeqkTMO8n94xhMRZlFmIQDokEQIgAq5U
-r1UQNnUExRh7ZT0kFbMfO9jKYZVlQdCL9Dn93vo=
+MIIJKAIBAAKCAgEAu0E+tR6wfOAJZ4lASzRUmvorCgbI5nQyvZl3WLu6ko2pcEnq


can you clarify the goal of updating this key?

The key is copied from the other PR because a new format was required. #1821

ah. got it. thanks for the link to that other PR. looks like i need to get up to speed with those details as well

pixelass · 2023-06-24T13:08:10Z

Thanks for taking a look. I'll adjust based on your comments. The key is copied from the other PR because a new format was required. #1821

wolfy1339 · 2023-10-29T02:05:52Z

Hi @pixelass woud you be interested in rebasing this off the beta branch, and continuing your work?

pixelass · 2023-11-12T06:04:05Z

Hi @pixelass woud you be interested in rebasing this off the beta branch, and continuing your work?

Hi, sorry for the silence. I've been very busy lately.
I won't have time to move all tests to vitest since

I'm on a tight schedule
I don't know vitest
Vite is in general not my stack

I can supoport with Jest, ESM and latest/best practices though and am happy to contribute.

wolfy1339 · 2023-11-12T16:09:45Z

No need to move tests over to vitest.

Jest is completely fine

AaronDewes · 2023-11-17T05:31:46Z

Our beta branch now has nearly all improvements here, and #1921 and #1922 implement full ESM.

wolfy1339 · 2023-12-04T00:43:24Z

Closing in favour of #1922

pixelass added 4 commits May 26, 2023 10:50

feat: esm only version

8bd3820

feat: remove require

d420b78

switch update-dotenv back to npm package

style: format files

43306ff

sort imports run prettier

test: esm testing

8d1d1a3

update jest to latest run esm compatible tests

pixelass added 2 commits May 26, 2023 21:04

feat: add node version check back

b295eb6

make previously removed version check esm compatible

fix: correct resolve import

1a59897

pixelass mentioned this pull request May 27, 2023

feat: Migrate to modern dependencies for v13 #1821

Merged

pixelass added 2 commits May 27, 2023 22:32

test: adjust rsa private key

98fe0f4

Thanks to: probot@0d74f5f

style: sort imports

dfcb92f

travi reviewed Jun 23, 2023

View reviewed changes

johnperez416 mentioned this pull request Nov 11, 2023

[Snyk] Upgrade pino from 6.11.3 to 8.16.0 johnperez416/probot#159

Closed

AaronDewes mentioned this pull request Nov 16, 2023

feat!: Full ESM #1922

Open

wolfy1339 closed this Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: esm only version #1825

feat: esm only version #1825

pixelass commented May 26, 2023 •

edited

welcome bot commented May 26, 2023

request-info bot commented May 26, 2023

travi commented Jun 23, 2023

travi left a comment

travi Jun 23, 2023

travi Jun 23, 2023

travi Jun 23, 2023

travi Jun 23, 2023

travi Jun 27, 2023

pixelass commented Jun 24, 2023 •

edited

wolfy1339 commented Oct 29, 2023

pixelass commented Nov 12, 2023 •

edited

wolfy1339 commented Nov 12, 2023

AaronDewes commented Nov 17, 2023

wolfy1339 commented Dec 4, 2023

	import "../lib/bin/probot-recieve.js";
	import "../lib/bin/probot-receive.js";

feat: esm only version #1825

feat: esm only version #1825

Conversation

pixelass commented May 26, 2023 • edited

welcome bot commented May 26, 2023

request-info bot commented May 26, 2023

travi commented Jun 23, 2023

travi left a comment

Choose a reason for hiding this comment

travi Jun 23, 2023

Choose a reason for hiding this comment

travi Jun 23, 2023

Choose a reason for hiding this comment

travi Jun 23, 2023

Choose a reason for hiding this comment

travi Jun 23, 2023

Choose a reason for hiding this comment

travi Jun 27, 2023

Choose a reason for hiding this comment

pixelass commented Jun 24, 2023 • edited

wolfy1339 commented Oct 29, 2023

pixelass commented Nov 12, 2023 • edited

wolfy1339 commented Nov 12, 2023

AaronDewes commented Nov 17, 2023

wolfy1339 commented Dec 4, 2023

pixelass commented May 26, 2023 •

edited

pixelass commented Jun 24, 2023 •

edited

pixelass commented Nov 12, 2023 •

edited