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: v5 #563

Merged
merged 30 commits into from
Jul 10, 2023
Merged

feat: v5 #563

merged 30 commits into from
Jul 10, 2023

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented May 21, 2023

BREAKING CHANGE: Drop support for NodeJS v14, v16
BREAKING CHANGE: Remove previews support for the REST API
BREAKING CHANGE: remove agent option from octokit.request()

* build(package): set minimal node version in engines field to v18
* build: update esbuild options to use Node 18
BREAKING CHANGE: Drop support for NodeJS v14, v16
@wolfy1339 wolfy1339 added Type: Feature New feature or request Type: Breaking change Used to note any change that requires a major version bump labels May 21, 2023
wolfy1339 and others added 2 commits June 13, 2023 20:40
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: wolfy1339 <[email protected]>
@wolfy1339

This comment was marked as resolved.

@wolfy1339 wolfy1339 marked this pull request as ready for review June 18, 2023 18:19
@wolfy1339
Copy link
Member Author

I'm having trouble getting the tests to pass. Can someone take a look?

@wolfy1339
Copy link
Member Author

wolfy1339 commented Jul 7, 2023

I was thinking of doing it that way.

Going through the dependencies of @octokit/core first to the very bottom of it's dependency graph, and going up from there.

Then @octokit/rest's dependency graph to the very bottom.

Then, octokit.js' dependency graph

We should catch most of the packages this way.

For those that didn't get caught, we follow the method outlined in the other discussion

@wolfy1339
Copy link
Member Author

wolfy1339 commented Jul 7, 2023

I have found this neat tool to visualize the dependency graph of NPM packages,
https://npmgraph.js.org/

@kfcampbell
Copy link
Member

Oh my gosh, that tool is awesome, this is super useful.

image

It looks like the next step is octokit/auth-token.js#354.

@wolfy1339
Copy link
Member Author

Alright, this seems good to go

Only thing that needs to be looked at are the tests

@wolfy1339 wolfy1339 requested review from kfcampbell and gr2m July 7, 2023 23:34
"@octokit/tsconfig": "^2.0.0",
"@types/fetch-mock": "^7.3.1",
"@types/jest": "^29.0.0",
"@types/lolex": "^5.1.0",
"@types/node": "^18.0.0",
"esbuild": "^0.18.0",
"fetch-mock": "^9.0.0",
"fetch-mock": "npm:@gr2m/[email protected]",
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to see this get merged into fetch-mock - any thoughts on what might be holding that up (since Sept 2022)?

The alias is fine, but it does seem like the project isn't even in maintenance mode - given the last commit to master was almost 2 years ago. Are there any alternatives to this mocking framework that would make for an easy migration and get us the functionality that we need here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know of any alternatives at the moment.

@nickfloyd
Copy link
Contributor

Only thing that needs to be looked at are the tests

I'll take a look at these today @wolfy1339. I assume it's just the ones you put a skip on in the commit tree, correct?

I'll post what I find.

@wolfy1339
Copy link
Member Author

Only thing that needs to be looked at are the tests

I'll take a look at these today @wolfy1339. I assume it's just the ones you put a skip on in the commit tree, correct?

I'll post what I find.

Yes, it's the ones I put a skip on

Comment on lines 6 to +7
const mock = fetchMock.sandbox().getOnce(
"https://api.github.com/",
"https://api.github.com/graphql",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update the test to something real? At least we should use POST /graphql. I think the GraphQL API supports get requests technically, but we always use POST /graphql.

Comment on lines 100 to +101
.getOnce(
"https://api.github.com/",
"https://api.github.com/graphql",
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, let's use POST

request: {
fetch: mock,
},
});

await octokit.request("/");
await octokit.request("/", {
await octokit.request("/graphql", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about it, maybe we should move these tests to test/graphql.test.ts now?

Copy link
Contributor

@nickfloyd nickfloyd Jul 10, 2023

Choose a reason for hiding this comment

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

I'll look into hoisting them out once I get past the skips. We could also differ it to another change set so we can move forward here. Not like the change would be hard, it just introduces more into something we've been trying to get out the door.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, this can totally be a follow up, like all of my comments

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.

I think this is good to go, all my comments above can be addressed in follow ups

@wolfy1339 wolfy1339 merged commit 0b4ad1e into main Jul 10, 2023
@wolfy1339 wolfy1339 deleted the beta branch July 10, 2023 19:11
@github-actions
Copy link
Contributor

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Uncaught ReferenceError: global is not defined
5 participants