search

stoically / webextensions-api-mock

WebExtensions API as sinon stubs
Mozilla Public License 2.0
5 stars 2 forks source link

Types - get 'runtime.connect()' to return a 'runtime.Port' #4

Closed mckenfra closed 4 years ago

mckenfra commented 4 years ago

See issue: https://github.com/stoically/webextensions-api-mock/issues/1 See stubs pr: https://github.com/stoically/webextensions-api-mock/pull/3 (and old types pr: https://github.com/stoically/webextensions-api-mock/pull/2)

This pull request is for the updates to the types in the types.d.ts file, to go along with the pull request mentioned above for the stubs.

As discussed on the other pull requests, I'm not sure how to test this. The generation works, and the build works without errors, but I don't have an IDE to test that the typescript interfaces/types are all now lined up correctly with the stubs!

mckenfra commented 4 years ago

Just to let you know, I'm currently rewriting this to use a common SchemaWalker class, shared with the stubs code. So maybe hold off your review for the time being.

Also I'm not totally happy with the types.d.ts file produced by this PR. I'm going to try to get it to produce similar output to the existing version on master, but with runtime.connect() returning a runtime.Port. Also the enum properties don't look right to me currently - e.g. windows.WindowType is an array: WindowsWindowType[], but shouldn't the type be WindowsWindowType for this property?

stoically commented 4 years ago

Just to let you know, I'm currently rewriting this to use a common SchemaWalker class, shared with the stubs code. So maybe hold off your review for the time being.

Great, thanks! I'll keep an eye on the PR then.

I'm going to try to get it to produce similar output to the existing version on master, but with runtime.connect() returning a runtime.Port

Sounds good.

Also the enum properties don't look right to me currently - e.g. windows.WindowType is an array: WindowsWindowType[], but shouldn't the type be WindowsWindowType for this property?

You're right, they're not correct. I didn't notice because the main consumer of this package, webextensions-api-fake, doesn't fake constant properties (yet), so yeah, good catch!

mckenfra commented 4 years ago

Ok I've finished the rewrite - the stubs and types code now share a common SchemaWalker class. Unfortunately this solution isn't quite as simple as I'd hoped - there's quite a lot of changed code, so may be difficult to review! Maybe I can try to simplify and reduce the code at some point. What might be good would be for you to check that the new generated types.d.ts output is correct: https://github.com/stoically/webextensions-api-mock/blob/5b8d566526fb44988b1a0f21b5b78981b1c54858/src/generated/types.d.ts

stoically commented 4 years ago

Looks great now. Would you mind rebasing on master and adding @types/chai, @types/sinon-chai as devdependencies?

stoically commented 4 years ago

Oh, and adding generate at the end of the build script would be appreciated too, so it runs in CI.

mckenfra commented 4 years ago

Oh, and adding generate at the end of the build script would be appreciated too, so it runs in CI.

You mean something like this in package.json?

"scripts": {
  "build": "npm-run-all lint test build:*",
  "build:tsc": "tsc -p tsconfig.build.json",
  "build:generate": "ts-node src/bin.ts",
  "build:copy": "copyup \"src/generated/*\" dist",
  "generate": "ts-node src/bin.ts",
  /* etc. */
}
stoically commented 4 years ago

Scratch that. I'll add it to the CI manually, since it might not be desired to generate when building.

mckenfra commented 4 years ago

Scratch that. I'll add it to the CI manually, since it might not be desired to generate when building.

ok cool - I was going to say if you add it to the end of the build command like you suggested, then thebuild:copy step would have been copying possibly out-of-date files from src/generated to dist/generated...

stoically commented 4 years ago

Right.

(Btw, I didn't meant to delete your comment, it was timing, apparently I hit delete the moment you commented; I wanted to delete my suggestion, should've just commented again, duh)

mckenfra commented 4 years ago

Right.

(Btw, I didn't meant to delete your comment, it was timing, apparently I hit delete the moment you commented; I wanted to delete my suggestion)

Haha - ok cool. Now rebased and added those dev dependencies: https://github.com/stoically/webextensions-api-mock/pull/4/commits/a0b07f55513872ea6d9692ab3dc211c4c53b0ea4

stoically commented 4 years ago

Will have to test this a bit from consuming packages before cutting a release, I'll try to do it soon!

stoically commented 4 years ago

Everything looking good! Released as v1.0.0 :tada:

mckenfra commented 4 years ago

Fantastic, thanks a lot! :smiley: :thumbsup: