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

Unfork mitm by using another workaround #118

Merged
merged 2 commits into from
Jun 29, 2020
Merged

Unfork mitm by using another workaround #118

merged 2 commits into from
Jun 29, 2020

Conversation

papandreou
Copy link
Member

@papandreou papandreou commented Jun 28, 2020

Use the suggestion posted here to avoid maintaining a fork of the mitm module.

Most of the diff is due to prettier's reformatting, so I recommend ignoring whitespace changes when viewing it.

@papandreou papandreou self-assigned this Jun 28, 2020
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.568% when pulling 7aa1d9b on tech/unforkMitm into 37b9a0b on master.

@papandreou papandreou requested a review from alexjeffburke June 28, 2020 00:51
@alexjeffburke
Copy link
Member

Oh, Cool! Nice that we can get the surface area down.

Once question though, rather than shift around that huge block fo code, could we take the opportunity to just have it as a function onRequest() {} or something and then wire it to mitm.on('request', createSerialized((onReqeust))? I'm happy to attempt the change and push it to this PR too :)

@papandreou
Copy link
Member Author

You're welcome to do that! It's not really a huge piece of code that can be shared, so I didn't feel it was a necessity.

@papandreou
Copy link
Member Author

@alexjeffburke, we can also merge this as-is and do further refactorings separately?

@alexjeffburke
Copy link
Member

Yeah, go for it and get a patch release out :)

@papandreou papandreou merged commit 45589b4 into master Jun 29, 2020
@papandreou papandreou deleted the tech/unforkMitm branch June 29, 2020 20:49
@papandreou
Copy link
Member Author

Released in 13.3.1

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.

3 participants