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

[MAINT]: Tracking issue for dropping support for Node v14/v16 and Fetch dispatch breaking changes #2486

Closed
63 tasks done
nickfloyd opened this issue Jun 22, 2023 · 9 comments
Closed
63 tasks done
Assignees
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@nickfloyd
Copy link
Contributor

nickfloyd commented Jun 22, 2023

Describe the need

We need a clear picture of where things are and what needs to ship when pushing through this breaking change.

Octokit JS Dependency Graph/Breaking Change map

Based on the discussions with @gr2m, @wolfy1339, @kfcampbell, and on the data above, the rough plan would be to:

Step 1:

Based on the data in the doc - find out if any of the repos that do not have beta branches or a PR for dropping node support for v14/v16 should have one. Why: This will give us 100% certainty that all packages (that need to) have been updated so that we can move forward with the Fetch breaking change:

Step 2

Go through all of the existing repos that have a beta branch and/or PR for dropping node support for v14/v16 and determine if we can merge (most likely, they are the ones blocked by core). Why: This will get all of the remaining change sets that can go out the door in a place where when we ship the change to core we can do it confidently.

Step 3

Remove request.agent option from request.js in a beta breaking change branch.

Step 4

Ship the core changes to either beta branches or main for packages that have already shipped node v14/16 - using the sequence detailed in the doc above

Step 5

Fast follow all of this with docs and examples. As @gr2m suggested:

Reference:

Note: As a group, we should stamp out a reasonably simple to consume dependency graph for all of our JS packages to help ease things like making breaking changes in the future.

@nickfloyd nickfloyd added Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR Status: Triage This is being looked at and prioritized labels Jun 22, 2023
@wolfy1339
Copy link
Member

I think @gr2m laid it out pretty well on the dependency graph in the previous discussion.

I think there's 2 branches currently that don't interfere with each other with regards to breaking changes:

  • @octokit/webhooks, and it's associated types/schema repository
  • Packages that depend on @octokit/core or @octokit/request

@nickfloyd nickfloyd changed the title [MAINT]: Tracking issue for dropping support for Node v14//v16 [MAINT]: Tracking issue for dropping support for Node v14/v16 and Fetch dispatch breaking changes Jun 22, 2023
@nickfloyd nickfloyd added the Type: Breaking change Used to note any change that requires a major version bump label Jun 22, 2023
@kfcampbell
Copy link
Member

@nickfloyd thank you for putting this together! That's an excellent summary and procedure.

@gr2m
Copy link
Contributor

gr2m commented Jun 22, 2023

Looks great! I'll help the best I can! Ping me anytime if my perspective would be helpful

@wolfy1339
Copy link
Member

Many issues are coming up about dropping Node 14/16, and node-fetch.

How can we be more explicit that this package (and other Octokit packages) does not support Node 14/16?

When it comes to the message logged from @octokit/request, it does seem misleading and confusing given we don't support Node 14/16 anymore, see #2495

Error: Global "fetch" not found. Please provide options.request.fetch to octokit or upgrade to node@18 or newer.

@nickfloyd
Copy link
Contributor Author

nickfloyd commented Jul 12, 2023

Solid call out @wolfy1339. I think the not found wording makes it seem like it's something octokit is supposed to take care of.

Perhaps we explicitly call out our lack of support there, something like:

Octokit ^3.x does not support NodeJS v16 and lower. You will need to either update to NodeJS ^18.x or pass fetch via options.request.fetch

IDK, perhaps something like that?

@gr2m
Copy link
Contributor

gr2m commented Jul 12, 2023

Maybe we could add a section to the README and link to it from the error, so we can provide a more thorough explanation and most importantly we can update whenever needed.

I'd say the availability of fetch is independent of the Node version. fetch is still experimental and it can be disabled with --no-experiemental-fetch. Also Node is not the only place where Octokit is run, I'm not sure if e.g. Electron or React Native have a global fetch method.

I'd do something like this:

fetch is not set. Please pass a fetch implementation as new Octokit({ request: { fetch }}). Learn more at https://github.com/octokit/octokit.js/#fetch-missing

@nickfloyd
Copy link
Contributor Author

I like @gr2m's approach here! @oscard0m you were asking how you might be able to lend a hand in all of this - is this something that you might be interested in picking up? I focusing on the actions libs (thankfully, @gr2m and @wolfy1339 have been incredibly helpful there) and will have to wrap that up before I can get to what they suggest above.

@oscard0m
Copy link
Member

oscard0m commented Jul 15, 2023

I like @gr2m's approach here! @oscard0m you were asking how you might be able to lend a hand in all of this - is this something that you might be interested in picking up? I focusing on the actions libs (thankfully, @gr2m and @wolfy1339 have been incredibly helpful there) and will have to wrap that up before I can get to what they suggest above.

#2497

@kfcampbell
Copy link
Member

I'm going to close this issue now as all of our breaking changes are rolled out! 🎉 🎉 🎉

Thank you all for driving these changes through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

No branches or pull requests

5 participants