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

Switch to TypeScript #31

Merged
merged 54 commits into from
Aug 17, 2021
Merged

Switch to TypeScript #31

merged 54 commits into from
Aug 17, 2021

Conversation

szmarczak
Copy link
Contributor

@szmarczak szmarczak commented Aug 6, 2021

TODO after this gets merged:

Closes #21

@szmarczak szmarczak marked this pull request as ready for review August 9, 2021 07:25
@szmarczak szmarczak requested a review from mnmkng August 9, 2021 07:25
@mnmkng mnmkng requested a review from B4nan August 9, 2021 08:04
Copy link
Member

@mnmkng mnmkng left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but overall I think we need to wait for @B4nan before merging.

package.json Outdated Show resolved Hide resolved
src/agent/transform-headers-agent.ts Outdated Show resolved Hide resolved
src/agent/transform-headers-agent.ts Outdated Show resolved Hide resolved
src/agent/transform-headers-agent.ts Outdated Show resolved Hide resolved
src/hooks/browser-headers.ts Outdated Show resolved Hide resolved

/**
* @param {object} options
*/
function optionsValidationHandler(options) {
export function optionsValidationHandler(options: unknown): void {
Copy link
Member

Choose a reason for hiding this comment

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

The possible options are quite well known, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They aren't known upon validation. We could type asserts options it T but it still doesn't matter as this is a hook.

src/index.ts Outdated Show resolved Hide resolved
test/custom-options.test.js Outdated Show resolved Hide resolved
test/proxy.test.js Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@mnmkng
Copy link
Member

mnmkng commented Aug 10, 2021

cc @vladfrangu

@szmarczak szmarczak requested a review from B4nan August 16, 2021 07:48
test/tsconfig.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/agent/transform-headers-agent.ts Outdated Show resolved Hide resolved
src/agent/wrapped-agent.ts Outdated Show resolved Hide resolved
src/agent/transform-headers-agent.ts Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
test/browser-headers.test.ts Outdated Show resolved Hide resolved
src/agent/transform-headers-agent.ts Outdated Show resolved Hide resolved

export function http2Hook(options: Options) {
if (options.http2 && (options.url as URL).protocol !== 'http:') {
// @ts-expect-error FIXME
Copy link
Member

Choose a reason for hiding this comment

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

this are still a lot of those FIXME type comments, do you plan to resolve all of them before merging the PR? if not, we should be probably more specific instead of just saying FIXME, at least in the actual code base (it's fine'ish in tests)

CHANGELOG.md Show resolved Hide resolved
src/hooks/proxy.ts Show resolved Hide resolved
@szmarczak
Copy link
Contributor Author

szmarczak commented Aug 16, 2021

this are still a lot of those FIXME type comments, do you plan to resolve all of them before merging the PR? if not, we should be probably more specific instead of just saying FIXME, at least in the actual code base (it's fine'ish in tests)

Fixed the two in index.ts and http2.ts.

The ones in main.test.ts are unfixable atm, see sindresorhus/got#1117 - I'll try to fix them later.

The PR is good to merge if there is no more questions.

@szmarczak
Copy link
Contributor Author

@B4nan Friendly ping :)

@szmarczak szmarczak merged commit 7e828a8 into master Aug 17, 2021
@szmarczak szmarczak deleted the ts branch August 17, 2021 10:19
@B4nan B4nan mentioned this pull request Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants