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

Types output - get 'runtime.connect' to return a 'runtime.Port' #2

Closed
wants to merge 1 commit into from

Conversation

mckenfra
Copy link
Contributor

@mckenfra mckenfra commented Jan 17, 2020

Re. issue: #1

I've rewritten the code a fair bit to handle nested interfaces and types, e.g. runtime.Port. Generation and build both run successfully.

I'd now like to test these changes against the following MAC pull request: mozilla/multi-account-containers#1613

What I'd like to do is to remove these lines: https://github.com/mozilla/multi-account-containers/blob/30a2601d35cc0d6aae1e0a681b69a0e6a995c751/test/helper.js#L22-L24

Then, I should be able to build successfully, using my updated version of webextensions-api-mock. I tried this by basically copying the dist dir from my modified version of webextensions-api-mock and overwriting node_modules/webextensions-api-mock/dist in my clone of multi-account-containers. But when I run test in the multi-account-containers clone, I get the same error as before - i.e. it doesn't think the return value of runtime.connect has any method named postMessage. But my updates to webextensions-api-mock should have meant that runtime.connect now returns a RuntimePort. Any ideas why this isn't working?

I also noticed that webextensions-api-mock/dist/generated/types.js is always an empty file. Maybe it should contain all of the compiled types from webextensions-api-mock/dist/generated/types.d.ts?

@stoically
Copy link
Owner

stoically commented Jan 17, 2020

Oh, nice, thanks for putting work into this!

Just a quick answer before I get around to test/review: This "only" modifies the type-generation which is relevant for TypeScript consumers, it doesn't change how the actual sinon stubs are generated, that happens in the index.ts - hence why you don't get actual sinon stubs returned when consuming it from the MAC-side. The types have to match the implementation; testing that would be best done in the tests by adding something that tries to e.g. access runtime.connect().name.

I also noticed that webextensions-api-mock/dist/generated/types.js is always an empty file

The types.d.ts is only supposed to provide the types for the lib, which are re-exported in the index.ts, so types.js doesn't need any content. Consuming that can look like this: import, usage

@mckenfra
Copy link
Contributor Author

Thanks for the explanation! Yeah, I've now discovered the tests, sorry I should have realised that before. I'm now adding in my extra tests for runtime.connect. In doing so, I've found that browser.runtime.connect() returns undefined. In types.d.ts this method is given the type of () => RuntimePort. But of course it hasn't been given any actual value to return! I need to try to figure out how to get this method to create and return a dummy instance of RuntimePort whenever it's called...

@mckenfra
Copy link
Contributor Author

Hang on... I may have had an idea how to do this

@stoically
Copy link
Owner

Aren't you interested in getting this merged any longer?

@mckenfra
Copy link
Contributor Author

mckenfra commented Jan 17, 2020

Yes - all I wanted to do was to rename my branch from runtime-connect to runtime-connect-types. So I used some git commands I found on google. And then of course it turns out those commands deleted this branch and therefore automatically closed the pull request! So I'll submit a new pull request from the branch with the new name.

@stoically
Copy link
Owner

Ah, understood. Tho, I think you can restore branches, but not rename branches for existing PRs, afaik.

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.

2 participants