OpenWPM icon indicating copy to clipboard operation
OpenWPM copied to clipboard

Remove our custom interface types and replace them with the @types/firefox-webext-browser ones

Open vringar opened this issue 4 years ago • 16 comments

Since the official types now have a lot more type definitions we can get rid of our own WebRequestOnBeforeSendHeadersEventDetails. This way we no longer need to maintain them manually. We should also check which other types we can just use from the library

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/e06e65d28fa1e3376fffc9ae08f85a74053518d3/types/firefox-webext-browser/index.d.ts#L4803-L4864

vringar avatar Nov 16 '20 12:11 vringar

I am a first time contributor. can you elaborate more on this issue?

nisan-abeywickrama avatar Nov 18 '20 07:11 nisan-abeywickrama

Hey @LordReigns Do you have any previous experience with TypeScript? If you don't I'd suggest you look at our other good-first-bug

As an overview, we currently define a lot of types for the WebExtension API ourselves, which is a lot of maintenance effort that we don't need to have. An example would be this: https://github.com/mozilla/OpenWPM/blob/512018f852a2b554b5b08a72936f835adf6b7110/openwpm/Extension/webext-instrumentation/src/types/browser-web-request-event-details.ts#L53-L99

If you compare the two types you'll notice that the official types have a lot more attributes. This task would ask, that you delete all of our type definitions that exist in a more complete form in @types/firefox-webext-browser

vringar avatar Nov 18 '20 09:11 vringar

Thank you @vringar for the response. Since my experience with typescript is low I'll be looking for another good-first-bug. If you don't mind could you recommend one that would suit a first time contributor.

Thank You. 😊

nisan-abeywickrama avatar Nov 18 '20 09:11 nisan-abeywickrama

Maybe have a look at #797

vringar avatar Nov 18 '20 14:11 vringar

If #800 looks interesting to @LordReigns, he can also take over that.

What is your opinion? @vringar I am happy to guide him through the necessary changes, I will also put 3 option into potential fixes where we can use WHERE clause

ankushduacodes avatar Nov 18 '20 14:11 ankushduacodes

After #797 #800 may be a good next step. But #797 is a high-impact and easy to do change, so I'd prioritize that.

vringar avatar Nov 18 '20 14:11 vringar

Thank you @vringar . I'll start working on the issue #797 I'm interested to work on issue #800 too, after finishing #797. @ankushduacodes . Your guidance would be very much appreciated.

Thank you all for your time 😊

nisan-abeywickrama avatar Nov 18 '20 14:11 nisan-abeywickrama

@vringar. I like to to work on this issue. As you explained delete all of our type definitions that exist in a more complete form in @types/firefox-webext-browser. Should the deleted ones be replaced?

Here is how I understood the task,


 interface _OnBeforeRequestDetails {
        /**
         * The ID of the request. Request IDs are unique within a browser session. As a result, they could be used to relate different events of the same request.
         */
        requestId: string;
        url: string;
        /** Standard HTTP method. */
        method: string;

For example: since this interface WebRequestOnBeforeRequestEventDetails exists in @types/firefox-webext-browser as above, I should delete the existing interface in this repository.

nisan-abeywickrama avatar Nov 20 '20 01:11 nisan-abeywickrama

Clarification

@vringar The replacement for the interface WebRequestOnBeforeRequestEventDetails is browser.webRequest.onBeforeRequest. How should I technically add this type. Do you mean that all the files where the previous interface was implemented should be replaced with the above or else only add it to the types ( If so could u explain the procedure).

Thank you

nisan-abeywickrama avatar Nov 21 '20 06:11 nisan-abeywickrama

Sorry for the delay. @vringar.

This is the approach I have taken,

example:

Consider the present interface WebRequestOnBeforeRequestEventDetails

I have replaced the above interface with the definition in firefox-webext-browser, as follows

import _OnBeforeRequestDetails  = browser.webRequest._OnBeforeRequestDetails;
export { _OnBeforeRequestDetails as WebRequestOnBeforeRequestEventDetails}

Can you confirm this approach, so I can finalize it.

Thank you.

nisan-abeywickrama avatar Nov 25 '20 13:11 nisan-abeywickrama

Hey, sorry for dropping the ball on this. This seems good :) Unfortunately I don't know too much about TypeScript so I'm unsure if it's the best way.

vringar avatar Nov 25 '20 13:11 vringar

Hey, sorry for dropping the ball on this. This seems good :) Unfortunately I don't know too much about TypeScript so I'm unsure if it's the best way.

Oh, Ok, Is there a test for this ? so I can make sure this doesn't cause any errors.

Thank You.

nisan-abeywickrama avatar Nov 26 '20 01:11 nisan-abeywickrama

As long as you can run ./scripts/build-extension.sh without any errors everything should be fine.

vringar avatar Nov 26 '20 09:11 vringar

Hey @LordReigns Hope you started off well in the new year :) Did you ever file a PR for this? From what we have discussed you seemed pretty close to having this done. Or do you need any help with anything.

vringar avatar Jan 29 '21 10:01 vringar

Hey @LordReigns Hope you started off well in the new year :) Did you ever file a PR for this? From what we have discussed you seemed pretty close to having this done. Or do you need any help with anything.

I couldn't work on it as I am busy with exams. Sorry about that. You can assign somebody else. @vringar

nisan-abeywickrama avatar Jan 29 '21 10:01 nisan-abeywickrama

Can you still push and file PR with what you've done so far? This is not urgent. Best of luck with your exams.

vringar avatar Jan 29 '21 10:01 vringar