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(deps): upgrade to got v10 #5469

Closed
wants to merge 38 commits into from
Closed

Conversation

szmarczak
Copy link

@szmarczak szmarczak commented Feb 12, 2020

  • json: true -> responseType: 'json'
  • body: {...} -> json: {...}
  • got.create({options: {}, handlers: [...]}) -> got.extend({handlers: [...]})
  • baseUrl -> prefixUrl (breaking change: prefixUrl will be always prepended, so set it to '' when you pass an absolute URL)

Please note that I might have missed something, so please double check.

Note: I don't have the time to correct the typings, but this PR is a good start.


TODO

  • refactor platform api options
  • refactor err.statusCode to res.response?.statusCode
  • fix tests

@viceice
Copy link
Member

viceice commented Feb 12, 2020

ok, thanks. I'll take this commit and push it to an internal branch and close this pr.

@viceice
Copy link
Member

viceice commented Feb 12, 2020

@szmarczak How can i extend the default Options type with custom properties?

We need this to pass custom properties to our got handlers.

@viceice
Copy link
Member

viceice commented Feb 12, 2020

@szmarczak How to update the retries function?

export default (numberOfRetries = NUMBER_OF_RETRIES): got.RetryFunction => (
_?: number,
err?: Partial<HTTPError>
): number => {
if (numberOfRetries === 0) {
return 0;
}
const { headers, statusCode } = err;
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
const isBanned = [TOO_MANY_REQUEST, SERVICE_UNAVAILABLE].includes(statusCode);
const delaySec = isBanned
? getBannedDelay(headers['retry-after'])
: getDefaultDelay(numberOfRetries);
// eslint-disable-next-line
numberOfRetries--;
const errorMessage = getErrorMessage(statusCode);
const message = `${errorMessage} Retry in ${delaySec} seconds.`;
logger.info(message);
return delaySec * 1000;
};

const json = true;
const retry = { retries: retriable() };
const headers = getHeaders();
const name = `${path}/${dependency}.json`;
const baseUrl = registry;
logger.trace({ dependency }, `RubyGems lookup request: ${baseUrl} ${name}`);
const response = (await got(name, { retry, json, baseUrl, headers })) || {
body: undefined,
};

@szmarczak
Copy link
Author

How can i extend the default Options type with custom properties?

I'd recommend going with the context option since Got 10 is confused which typing it should choose when you cast the options as GotOptions.

@szmarczak
Copy link
Author

How to update the retries function?

Currently I'm not able to rewrite it for you, but will do in an hour. Here's the docs:

https://github.com/sindresorhus/got/blob/master/readme.md#retry

@viceice
Copy link
Member

viceice commented Feb 12, 2020

ok, found context and use it, now i only need help at retries.

@viceice
Copy link
Member

viceice commented Feb 12, 2020

@szmarczak can you retarget the pr to szmarczak/got-10 when you pushed your changes? Thanks.

@szmarczak
Copy link
Author

can you retarget the pr to szmarczak/got-10 when you pushed your changes?

Did you mean "when you push"? If so, then sure, will do :)

Unfortunately I'm not able to convert the retries logic today. Gonna do that tomorrow. I hope you don't mind the waiting.

@viceice
Copy link
Member

viceice commented Feb 12, 2020

yes, i can't retarget pr if my branch is infornt of yours 😅 No hurry, i'm done with most type changes. Now fixing some tests, which i won't finish today.

@rarkins How urgnet is this upgrade? I've seen the got issue above.

@viceice
Copy link
Member

viceice commented Feb 12, 2020

@szmarczak Another type difficulty. your types say Options.context is always defined, but our tests failed on that. Maybe contect should be defined as optional?

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

needs retargetting to szmarczak/got-10 🙃

@viceice
Copy link
Member

viceice commented Feb 12, 2020

ok 9 tests to got. 😴 🙃

https://app.circleci.com/jobs/github/renovatebot/renovate/20165/tests

@szmarczak
Copy link
Author

Another type difficulty. your types say Options.context is always defined

If think you're confusing Options with NormalizedOptions. According to this piece of code it is always defined. If it isn't just please point it out at some piece of code, I'd love to help :)

yes, i can't retarget pr if my branch is infornt of yours

Neither can I. But will do when I pull your changes and commit mine.

@rarkins
Copy link
Collaborator

rarkins commented Feb 13, 2020

@rarkins How urgnet is this upgrade? I've seen the got issue above.

Not urgent. I think I've managed to fix the problem in got@9

@viceice
Copy link
Member

viceice commented Feb 13, 2020

@rarkins ok

@szmarczak I've pushed my changes to your branch now, so we can see what's missing.

@viceice viceice changed the title Upgrade to Got 10 feat(deps): upgrade to got v10 Feb 13, 2020
@viceice
Copy link
Member

viceice commented Feb 13, 2020

@viceice
Copy link
Member

viceice commented Feb 13, 2020

@szmarczak Can nock be the problem for the failing tests?

All failing tests use nock for mocking net requests.

@viceice
Copy link
Member

viceice commented Feb 13, 2020

Ok, i have to refactor all got wrapper for platforms, as they mix options the old way

@szmarczak
Copy link
Author

Can nock be the problem for the failing tests?

A very small chance. I don't think so.

I'll fix the retries today in the afternoon when I'm back from school.

@viceice
Copy link
Member

viceice commented Feb 17, 2020

I can't find the cause for the failing maven / sbt tests

@viceice
Copy link
Member

viceice commented Feb 27, 2020

I've fixed some inconsistencies. I'll porting them to a seperate pr.

@viceice
Copy link
Member

viceice commented Feb 27, 2020

Fixed all tests 💪

Maybe some coverage is now missing. I'll add tests then.

@viceice
Copy link
Member

viceice commented Feb 27, 2020

Strange, I don't get the lint errors on my local windows 10. 🤔

@JamieMagee
Copy link
Contributor

@szmarczak I think this PR is mostly superseded by #5841. Unfortunately I don't think it's worth rebasing or merging master with these changes. Instead, I think starting from current master would be easier. Sorry about that!

If you disagree, leave a comment and I can reopen this issue.

@JamieMagee JamieMagee closed this Apr 12, 2020
@szmarczak
Copy link
Author

That's ok, Got 11 is about to be released - compared to Got 10 it is a huge improvement. The API is almost the same.

@viceice viceice mentioned this pull request Jul 10, 2020
2 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants