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

feat: v13 #1874

Merged
merged 52 commits into from Jan 25, 2024
Merged

feat: v13 #1874

merged 52 commits into from Jan 25, 2024

Conversation

wolfy1339
Copy link
Collaborator

@wolfy1339 wolfy1339 commented Oct 29, 2023

BREAKING CHANGE: Drop support for NodeJS < 18
BREAKING CHANGE: replace node-fetch with the Fetch API
BREAKING CHANGE: default webhookPath is now /api/github/webhooks
BREAKING CHANGE: probot receive now only supports payloads in JSON format, previously also (unintionally) allowed JS.

closes #1643

BREAKING CHANGE: Drop support for NodeJS < 18
BREAKING CHANGE: replace `node-fetch` with the Fetch API
@wolfy1339 wolfy1339 marked this pull request as ready for review October 29, 2023 02:56
@wolfy1339 wolfy1339 requested a review from a team as a code owner October 29, 2023 02:56
@Uzlopak

This comment was marked as outdated.

fix linting of extensions.md
@Uzlopak

This comment was marked as outdated.

@AaronDewes

This comment was marked as outdated.

Remove @tsconfig/node10 from devDepedencies as we are streamling with @octokit/tsconfig
src/octokit/get-authenticated-octokit.ts Outdated Show resolved Hide resolved
src/octokit/get-authenticated-octokit.ts Outdated Show resolved Hide resolved
Uzlopak and others added 2 commits November 12, 2023 09:50
chore: remove semver as dependency
* Set up code scanning

This sets up CodeQL to automatically detect possible security problems.

* Add more queries
test/probot-octokit.test.ts Fixed Show resolved Hide resolved
test/probot-octokit.test.ts Fixed Show fixed Hide fixed
test/probot-octokit.test.ts Fixed Show fixed Hide fixed
@AaronDewes
Copy link
Member

AaronDewes commented Nov 14, 2023

What I'd like to do before merge (Am I missing anything here?):


After the release:

Since Node 18, Headers is a global.
@gr2m
Copy link
Contributor

gr2m commented Dec 6, 2023

Huh, we should probably put this into Octokit 🤔 unless it has bad performance implications, but I'd only happen in the constructor so I don't think so? I don't mind leaving this in Probot to avoid the breaking change

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 6, 2023

I dont have any clue, why the aliasLog was removed in the first place. Or why its type was called "DeprecatedLogger". I assume that there are some deeper thoughts behind that. Would be glad if you or @wolfy1339 could take care of it, because it is 1 a.m. in the morning and i have to grab some sleep. :D

@wolfy1339
Copy link
Collaborator Author

4e7884e

This is the source of the type DeprecatedLogger

This commit deprecated .logging in favor of .log
2bacd14

So it seems that the name is lingering from old times, and the aliasLog function is remnant from the revert of the depreciation, but the name wasn't updated

@gr2m
Copy link
Contributor

gr2m commented Jan 14, 2024

Hey shall we ship v13? Anything that is left to do from your perspective?

@wolfy1339
Copy link
Collaborator Author

There is the log binding issue that should be addressed.

Other than that, nothing seems to be blocking the release

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 16, 2024

I could not reproduce the log binding error in a test. So it was not clear if the problem would be solved with that PR.
I would say, lets merge and release it as v13.
People will report if this log binding issue was not only a fluke.
Or we decide to merge the log binding PR, which u closed rn, but without a test...

@AaronDewes
Copy link
Member

I dont have any clue, why the aliasLog was removed in the first place. Or why its type was called "DeprecatedLogger". I assume that there are some deeper thoughts behind that. Would be glad if you or @wolfy1339 could take care of it, because it is 1 a.m. in the morning and i have to grab some sleep. :D

I think I did that and removed it because it was called DeprecatedLogger, so I just thought it was a backwards-compat thing and was supposed to be removed in a major release.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 16, 2024

So should we merge #1939 just for the sake of it?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 18, 2024

I merged it. Maybe we can finally release :D

Copy link
Member

@AaronDewes AaronDewes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's go 🚀

Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfy1339 all yours!

@wolfy1339 wolfy1339 merged commit 948a1b7 into master Jan 25, 2024
21 checks passed
@wolfy1339 wolfy1339 deleted the beta branch January 25, 2024 21:03
Copy link

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

nikku added a commit to nikku/wuffle that referenced this pull request Jan 31, 2024
This is necessary with probot@13, see probot/probot#1874 (comment).
nikku added a commit to nikku/wuffle that referenced this pull request Jan 31, 2024
This is necessary with probot@13, see probot/probot#1874 (comment).
@nikku
Copy link

nikku commented Jan 31, 2024

This is a breaking change and hit my during my probot app update. General search was not helpful, so the only way to find it was to skim through all conversations to find that comment.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Feb 1, 2024

@nikku
We tried to mitigate the issue via #1939 but it seems we were not successful. If you have an idea on why this happens, we are glad to accept a PR from you. I dont have a clue, why this happens or how we can fix it.

@gr2m
Copy link
Contributor

gr2m commented Feb 9, 2024

I finally updated the WIP app to probot v13, thanks to nock@beta which now supports native fetch mocking 🎉 which blocked me. One thing I observed is that tests now take forever, as if the way I disabled throttling no longer works. Did anyone else observe something similar? I didn't have time to dig deeper into it:
wip/app#631

UPDATE: had a quick look, throttling definitely remains enabled although I disable it e.g. here:
https://github.com/wip/app/blob/70b9da8a45ee39be4c107692f6fadd90b1bacad5/test/integration/pro-plan-test.js#L34

Not sure why yet, might be a ProbotOctokit or even an Octokit bug with how defaults are set

@travi
Copy link
Member

travi commented Feb 10, 2024

Did anyone else observe something similar?

there is an open report here: #1972

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some payloads may not be parsed correctly onAny doesn't provide types for context.log
9 participants