chore: migrate src/Connection to TypeScript
#5694
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mathiasbynens
approved these changes
Apr 20, 2020
|
Yay for modules! |
jackfranklin
force-pushed
the
migrate-connection
branch
from
4bb69a5
to
34d7904
Compare
This commit migrates `src/Connection` to TypeScript. It also changes its exports to be ESM because TypeScript's support for exporting values to use as types via CommonJS is poor (by design) and so rather than battle that it made more sense to migrate the file to ESM. The good news is that TypeScript is still outputting to `lib/` as CommonJS, so the fact that we author in ESM is actually not a breaking change at all. So going forwards we will: * migrate TS files to use ESM for importing and exporting * continue to output to `lib/` as CommonJS * continue to use CommonJS requires when in a `src/*.js` file I'd also like to split `Connection.ts` into two; I think the `CDPSession` class belongs in its own file, but I will do that in another PR to avoid this one becoming bigger than it already is. I also turned off `@typescript-eslint/no-use-before-define` as I don't think it was adding value and Puppeteer's codebase seems to have a style of declaring helper functions at the bottom which is fine by me. Finally, I updated the DocLint tool so it knows of expected method mismatches. It was either that or come up with a smart way to support TypeScript generics in DocLint and given we don't want to use DocLint that much longer that didn't feel worth it.
jackfranklin
force-pushed
the
migrate-connection
branch
from
509d4e5
to
f0bcdbb
Compare
jackfranklin
commented
Apr 20, 2020
| const id = this._connection._rawSend({ | ||
| sessionId: this._sessionId, | ||
| method, | ||
| params: params || {} |
There was a problem hiding this comment.
hey @mjzffr I think there might be a Firefox bug here with the fact that it's expecting params to be present. Without this || {} some Firefox specs fail like so:
1) Page.click
should not hang with touch-enabled viewports:
Error: Protocol error (Page.reload): (destructured parameter) is undefined reload@chrome://remote/content/domains/content/Page.jsm:135:1
execute@chrome://remote/content/domains/DomainCache.jsm:99:25
receiveMessage@chrome://remote/content/sessions/ContentProcessSession.jsm:69:45
MessageListener.receiveMessage*ContentProcessSession@chrome://remote/content/sessions/ContentProcessSession.jsm:27:25
@chrome://remote/content/sessions/frame-script.js:12:1
at Promise (lib/Connection.js:147:63)
at new Promise (<anonymous>)
at CDPSession.send (lib/Connection.js:146:16)
at Page.reload (lib/Page.js:630:26)
at Page.<anonymous> (lib/helper.js:82:27)
at Page.setViewport (lib/Page.js:776:24)
at process._tickCallback (internal/process/next_tick.js:68:7)
-- ASYNC --
at Page.<anonymous> (lib/helper.js:81:19)
at Context.it (test/click.spec.js:207:16)
at process._tickCallback (internal/process/next_tick.js:68:7)
However Chrome is fine. I think that maybe FF is expecting params where it shouldn't? This is an easy workaround for now but might be worth looking into. I also might have misunderstood the issue so sorry if that's the case. Also pinging @mathiasbynens for any thoughts.
There was a problem hiding this comment.
@jackfranklin Thanks, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1631570
jackfranklin
force-pushed
the
migrate-connection
branch
from
fc33391
to
a1ece92
Compare
This was referenced May 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit migrates
src/Connectionto TypeScript. It also changes itsexports to be ESM because TypeScript's support for exporting values to
use as types via CommonJS is poor (by design) and so rather than battle
that it made more sense to migrate the file to ESM.
The good news is that TypeScript is still outputting to
lib/asCommonJS, so the fact that we author in ESM is actually not a breaking
change at all.
So going forwards we will:
lib/as CommonJSsrc/*.jsfileI'd also like to split
Connection.tsinto two; I think theCDPSessionclass belongs in its own file, but I will do that inanother PR to avoid this one becoming bigger than it already is.
I also turned off
@typescript-eslint/no-use-before-defineas I don'tthink it was adding value and Puppeteer's codebase seems to have a style
of declaring helper functions at the bottom which is fine by me.