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
feat(TypeScript): Move DeviceDescriptors to TS #5595
Conversation
@mathiasbynens see what you think - I don't want to ship this anytime soon but thought I'd see what the changes looked like. |
I've made a PR a while ago which got ignored: After seeing the work being merged these days, you may find some of the those changes useful. |
No longer exporting the array is also a breaking change. I think that's fine for the reasons you point out, but we should just be aware of it. |
@AviVahl I did see that indeed, thanks! It will definitely be a useful reference as we try to incrementally migrate over 👍 |
83b7455
to
d6a4819
Compare
src/DeviceDescriptors.ts
Outdated
[name: string]: Device | ||
} | ||
|
||
const devicesMap: DevicesMap = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const devicesMap: DevicesMap = {} | |
const devicesMap: DevicesMap = {}; |
src/DeviceDescriptors.ts
Outdated
deviceScaleFactor: number; | ||
isMobile: boolean; | ||
hasTouch: boolean; | ||
isLandscape: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isLandscape: boolean | |
isLandscape: boolean; |
right?
Note to self: I'll rephrase the commit message upon merging in light of the following:
This is technically a breaking change (although for the reasons you provide it seems fine in this case).
Given the above, it would be "a larger breaking change". |
This commit moves `src/DeviceDescriptors` to be authored in TypeScript. This file was chosen due to its simplicity so that we can focus on getting a mixed JS/TS codebase playing nicely before migrating the more complex files. The file itself was a bit odd: although the array of devices was exported via `module.exports` that was never referenced by any consumers; each device was also exported via `module.exports[name] = device` and that is how it's consumed. The Puppeteer docs suggest using it like so: ```js puppeteer.devices['iPhone 6'] ``` So instead of exporting the array and then setting a bunch of properties on that, we instead define the array and export an object of keys where each key is a device. This is a breaking change (see the footer for details). Rather than export an object I'd much rather export a Map, but that would be a larger breaking change and I'm keen to avoid those for the time being. Note that we have to use special TypeScript specific syntax for the export that enables it to work in a CommonJS codebase [1] but again I'd rather this than move to ESM at this time. TypeScript still outputs CommonJS into `lib/` as you would expect. BREAKING CHANGE: We no longer export an array of devices, so any users relying on doing: ```js puppeter.devices.forEach(...) ``` Will now see a breakage. The fix is to use `Object.{keys/entries/values}` to iterate instead. [1]: https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require
d6a4819
to
9a12c04
Compare
doclint seems to be timing out on one of the bots. Could you please take a look? |
This commit moves
src/DeviceDescriptors
to be authored in TypeScript.This file was chosen due to its simplicity so that we can focus on
getting a mixed JS/TS codebase playing nicely before migrating the more
complex files.
The file itself was a bit odd: although the array of devices was
exported via
module.exports
that was never referenced by anyconsumers; each device was also exported via
module.exports[name] = device
and that is how it's consumed. The Puppeteer docs suggest usingit like so:
So instead of exporting the array and then setting a bunch of properties
on that, we instead define the array and export an object of keys where
each key is a device.
Rather than export an object I'd much rather export a Map, but that
would be a breaking change and I'm keen to avoid those for the time
being.
Note that we have to use special TypeScript specific syntax for the
export that enables it to work in a CommonJS codebase 1 but again I'd
rather this than move to ESM at this time. TypeScript still outputs
CommonJS into
lib/
as you would expect.