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

feat(TypeScript): Move DeviceDescriptors to TS #5595

Merged
merged 2 commits into from Apr 14, 2020

Conversation

jackfranklin
Copy link
Collaborator

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:

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.

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.

@jackfranklin
Copy link
Collaborator Author

@mathiasbynens see what you think - I don't want to ship this anytime soon but thought I'd see what the changes looked like.

@AviVahl
Copy link
Contributor

AviVahl commented Apr 7, 2020

I've made a PR a while ago which got ignored:
#5233

After seeing the work being merged these days, you may find some of the those changes useful.

@mathiasbynens
Copy link
Member

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.

@jackfranklin
Copy link
Collaborator Author

@AviVahl I did see that indeed, thanks! It will definitely be a useful reference as we try to incrementally migrate over 👍

@jackfranklin jackfranklin force-pushed the move-device-descriptions-to-typescript branch from 83b7455 to d6a4819 Compare April 14, 2020 08:26
[name: string]: Device
}

const devicesMap: DevicesMap = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const devicesMap: DevicesMap = {}
const devicesMap: DevicesMap = {};

deviceScaleFactor: number;
isMobile: boolean;
hasTouch: boolean;
isLandscape: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isLandscape: boolean
isLandscape: boolean;

right?

@mathiasbynens
Copy link
Member

Note to self: I'll rephrase the commit message upon merging in light of the following:

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 technically a breaking change (although for the reasons you provide it seems fine in this case).

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.

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
@mathiasbynens
Copy link
Member

doclint seems to be timing out on one of the bots. Could you please take a look?

@mathiasbynens mathiasbynens merged commit 88d843d into master Apr 14, 2020
@mathiasbynens mathiasbynens deleted the move-device-descriptions-to-typescript branch April 14, 2020 09:55
@mathiasbynens mathiasbynens added this to the TypeScript migration milestone Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants