update to v16

[gr2m]

v16 is still work in progress. Tests should pass once https://github.com/octokit/rest.js/pull/1094 is released.

For v16, we should be able to get rid of requiring @octokit/rest/lib/routes. With v16, you can get the request method with github.repos.get.endpoint().method

[pvdlg]

@gr2m shouldn't the dependency to @octokit/rest bet set to ^16.0.0 in this PR rather than ^15.18.0?

[gr2m]

Yes :) The update to ^15.18.0 first is the recommended upgrade path, it deprecates all the methods that are removed in 16.

I’m not quite sure why the tests failed, I thought I had it fixed locally. What changed there is that the request to add labels no longer sends an array in the request root

["label1", "label2"]

But instead has a namespace

{"labels":["label1", "label2"]}

Let me have another look

[gr2m]

Okay it was just a space. I’ll now work on upgrading to v16, there are a few other changes needed because you used internals that have changed now. It should all become much easier with v16

[gr2m]

Hold on, I would not merge it with v15.8.0, it will log a lot of deprecation messages, I’d try to directly upgrade to v16

[gr2m]

I think I’ll need your help, the implementation is a little confusing right now.

In get-client.js:84 you pass in the limitKey as first argument to getLimitKey. limitKey is something like "core.write" or "search"

github/lib/get-client.js

Lines 71 to 84 in fe2a3fc

	const handler = (globalThrottler, limitKey) => ({
	/**
	* If the target has the property as own, determine the rate limit based on the property name and recursively wrap the value in a `Proxy`. Otherwise returns the property value.
	*
	* @param {Object} target The target object.
	* @param {String} name The name of the property to get.
	* @param {Any} receiver The `Proxy` object.
	*
	* @return {Any} The property value or a `Proxy` of the property value.
	*/
	get: (target, name, receiver) =>
	Reflect.apply(Object.prototype.hasOwnProperty, target, [name])
	? new Proxy(target[name], handler(globalThrottler, getLimitKey(limitKey, name)))
	: Reflect.get(target, name, receiver),

Now the getLimitKey function says the first argument should be endpoint, e.g. "repos" for "github.repos.get"

github/lib/get-client.js

Lines 45 to 59 in fe2a3fc

	/**
	* Get the limiter identifier associated with a client API call.
	*
	* @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
	* @param {String} command The client API command (for example the command for a call to `github.repos.get` is `get`).
	*
	* @return {String} A string identifying the limiter to use for this `endpoint` and `command` (e.g. `search` or `core.write`).
	*/
	const getLimitKey = (endpoint, command) => {
	return endpoint
	? [endpoint, RATE_LIMITS[endpoint] && getAccess(endpoint, command)].filter(Boolean).join('.')
	: RATE_LIMITS[command]
	? command
	: 'core';
	};

Did something get mixed up here?

For v16, we no longer need to load the ROUTES from @octokit/rest like we do right now with const GH_ROUTES = require('@octokit/rest/lib/routes');. You can get the endpoint method’s request method with

octokit.repos.get.endpoint().method

I also hope that we no longer need the Proxy to wrap the endpoint methods, instead we could create a proper Octokit plugin for throttling: https://github.com/octokit/rest.js/issues/1099. We could extract the logic from @semantic-release/github into such a plugin, what do you think?

Instead of using a Proxy, we can do something like this:

octokit.hook.wrap('request', (request, options) {
  return throttle(request.bind(null, options))
})

see https://github.com/octokit/rest.js#hooks

[pvdlg]

The function get in the handler will be called recursively. So if you call github.issues.create the get function will be called once with issues and once with create. On each call we create a proxy, and we need to pass the limitKey to the new handler so when the get the is called you can now the parent object.
In other words, if you call github.issues.create, in the second call to get, name is create but you also need to know that create was called on issues.

We don't only throttle, we also retry some requests (the retried requests are also throttled).

If we were to create a proper Octokit plugins I would include more things that what we do here:

• Allow to configure everything (throttle rate, number of retry, which request to retry, etc...)
• Handle X-RateLimit-Limit, X-RateLimit-Remaining and X-RateLimit-Reset
• Find a better/more accurate way to handle throttling of request that trigger a notification (rather than blindly throttling at 3 seconds)

So I can work on migrating the Proxy thing to a plugin with the features we currently have (and keep the code here). Regarding the implementation of a proper plugin, I don't think I would have enough time, and there is other semantic-release things higher on my priority list.

[gr2m]

I can work on migrating the Proxy thing to a plugin with the features we currently have (and keep the code here)

sounds good 👍

[pvdlg]

So maybe we can update the dependency to 16.0.0 and merge this PR. I'll work on replacing the Proxy in a different PR.

[gr2m]

I would remove this line as it might break any time

https://github.com/semantic-release/github/blob/fe2a3fc7a5f777210c3e6206ebfc13c720844447/lib/get-client.js#L10

I changed it to

const GH_ROUTES = require('@octokit/rest/plugins/rest-api-endpoints/routes');

to work with the current v16, but it’s an implementation detail and might change anytime.

Use octokit.repos.get.endpoint().method to get the default http method for the endpoint methods instead

[semantic-release-bot]

🎉 This PR is included in version 5.2.2 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[pvdlg]

For info there was indeed a bug with getLimitKey and the way we determine if an endpoint is a read or write. See #128