-
Notifications
You must be signed in to change notification settings - Fork 177
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
ESM support #569
ESM support #569
Conversation
@mattwynne is attempting to deploy a commit to the Probot Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like where this is headed. it looks like there are quite a few formatting changes included that are unrelated to the ESM transition, though. we dont have the project set up with a great lint or auto-formatting story at this point, but improving that situation seems separate enough from this effort that i think i would prefer those types of changes be in a separate PR. would backing those out of this change be a huge pain?
Gah, sorry about that. I thought I'd configured my editor to follow the I agree that's going to be confusing to try and review. Let me take a look. |
since we dont have things configured to be very strict yet, i expect that there are likely things that are not currently formatted consistently. i agree with the approach of getting a better handle on that and using that improvement to simplify this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a question about the settings export and a small cleanup item
@mattwynne would you prefer i squash the history of this branch, or do you think it is worth keeping the story of the progression? |
👍 for a squash |
I can also rebase and tidy it up - all those dependabot commits are just from a sloppy merge. |
either way is ok with me. we can go with whichever you'd be most happy with |
There you go, there's one nice clean commit for the main branch. In general I'm totally happy to have my commits from a branch squashed when they go into main. I tend to make a lot of small commits when I'm working on something, but that kind of detail usually just creates clutter in the narrative of the main branch. In Cucumber we only ever merge with squash. |
This change broke my deployment and I'm not sure how to get it working again. I was running a very simple Docker image, deployed to my own Kube cluster, that contains this simple package.json: {
"name": "@taas/github-settings",
"version": "0.0.0-semantic-release-0",
"private": true,
"scripts": {
"start": "probot run probot-settings"
},
"dependencies": {
"probot": "12.2.2",
"probot-settings": "probot/settings"
}
} After this PR, though, my deployment started to fail with the following error:
I think because Probot itself isn't an ESM module. So, the I tried creating a simple import Probot from 'probot'
import settingsBot from 'probot-settings'
await Probot.run(settingsBot) But that failed with this error:
The only solution I've found, so far, is to pin |
Oh dear! I wonder if you need to change your I'm inclined to think Probot does work OK with |
Ah, thank you! I added |
Great! Thanks for checking back to let us know. |
No worries, I can imagine this isn't the yak you intended to be shaving today 😁 I tried to make a minimal reproducible example, here, which seems to work OK. Any thoughts on how that setup is different to yours? |
Sounds like the next logical step. 👍 If you had any time to fork it and do that I'd appreciate it. |
Very strangely, though, I still end up with this error even if I use the |
I think those are false positives. If you look at the log output there's still an error but it's buried in an unhandled promise: |
Oh, right, of course. Because unhandled rejections didn't become fatal until 16. That explains why my kube deployment still errors out: the readiness check fails. |
That ought to do it. It's really strange to me that this is a problem, though. Your PR was two weeks ago -- it seems bizarre that I'd be the only one complaining if this the breaking change that it looks like. But maybe people don't deploy their own settings bot very often? I really like the changes in ESM, but the migration process has been an intermittent headache for months now. 😢 |
This reverts commit 9fb82c4.
This reverts commit 67bfd13 Co-authored-by: Matt Wynne <[email protected]> Co-authored-by: nitrocode <[email protected]> BREAKING CHANGE: the project is now ESM only
Switch over to
import
fromrequire
.I followed https://jestjs.io/docs/ecmascript-modules for Jest config.