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

fix: wrap onRequestFinished to use promises #250

Merged
merged 1 commit into from
Dec 10, 2020
Merged

fix: wrap onRequestFinished to use promises #250

merged 1 commit into from
Dec 10, 2020

Conversation

dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Dec 7, 2020

Fixes #249.

This updates browser.devtools.network.onRequestFinished to emit an
object with a promisified getContent() property. This brings the
polyfill implementation in line with Firefox's implementation, although
documentation is still inaccurate at the moment.

Also updates some out of date documentation with makeCallback().

I tested this in my own Chrome extension and it appears to work as expected.

@dgp1130
Copy link
Contributor Author

dgp1130 commented Dec 7, 2020

CI seems to be having a versioning error with Chrome that seems to be unrelated to this PR?

src/browser-polyfill.js Outdated Show resolved Hide resolved
src/browser-polyfill.js Show resolved Hide resolved
test/test-onRequestFinished.js Show resolved Hide resolved
@Rob--W
Copy link
Member

Rob--W commented Dec 8, 2020

CI seems to be having a versioning error with Chrome that seems to be unrelated to this PR?

I'll take care of that in #251

test/test-onRequestFinished.js Show resolved Hide resolved
test/test-onRequestFinished.js Outdated Show resolved Hide resolved
test/test-onRequestFinished.js Outdated Show resolved Hide resolved
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

LGTM thanks. I'll merge it after the other PR that unbreaks the build is merged.

@Rob--W
Copy link
Member

Rob--W commented Dec 10, 2020

The other PR got approved and merged. Could you rebase and update the PR so we can get a green CI build?

Fixes #249.

This updates `browser.devtools.network.onRequestFinished` to emit an
object with a promisified `getContent()` property. This brings the
polyfill implementation in line with Firefox's implementation, although
MDN documentation is still inaccurate at the moment.

Also updates some out of date documentation with `makeCallback()` and
`wrapAsyncFunction()`.
@dgp1130
Copy link
Contributor Author

dgp1130 commented Dec 10, 2020

Rebased and got a green CI, thanks for fixing that. Should be ready to merge.

@Rob--W
Copy link
Member

Rob--W commented Dec 10, 2020

Thanks for the report and patch!

@Rob--W Rob--W merged commit 716c90b into mozilla:master Dec 10, 2020
@dgp1130 dgp1130 deleted the get-content branch December 10, 2020 22:26
@caitmuenster
Copy link

Thanks so much for the patch, @dgp1130! Your contribution has been added to our recognition wiki.

Take care!

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.

browser.devtools.network.onRequestFinished does not promisify getContent()
3 participants