feat(types): expose typedefs to consumers #6745
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
This PR replaces #6289 with a simpler approach to types where we compile them all alongside the compiled code (so for every `foo.js` that is generated, we generate `foo.d.ts`). By default that's not enough, as when you `import puppeteer from 'puppeteer'` in Node land you import `cjs-entry.js`, so we also create `cjs-entry.d.ts` which TypeScript will then pick up. This type file tells TypeScript that the thing that `cjs-entry.js` exposes is an instance of the `Puppeteer` class, which then hooks all the types up. The previous PR (#6289) tried to merge all our typedefs into one big file via API Extractor, but this isn't really necessary. good experience to the developer. One important consideration is that you _could_ consider this a breaking change. I'm not sure how likely it is, but it could cause problems for: * Projects that didn't have any type info for Puppeteer and treated all its exports as `any` may now start having legitimate type failures. * Projects that depend on the `@types/puppeteer` package may have issues if they now swap to use this one and the types aren't quite aligned. In addition, once we do ship a release with this change in, it will mean that we have to treat any changes to any type definitions as release-note-worthy, and any breaking changes to type definitions will need to be treated as breaking code changes (nearly always a breaking type def means a breaking change anyway), but it's worth considering that once we expose these typedefs we should treat them as first class citizens of the project just like we would with the actual source code. I also fully expect to have some bugs with our types, or have users create issues/PRs to change our types, but I think that's a good thing and it should settle down. I've tested this locally by creating a new package, linking Puppeteer via `npm link puppeteer` and then checking that VSCode correctly shows me the right things when I use `Go to Definition` on something that comes from the Puppeteer source code.
jackfranklin
force-pushed
the
ship-type-defs
branch
from
af4959f
to
e170a6d
Compare
|
|
mathiasbynens
approved these changes
Jan 13, 2021
AviVahl
reviewed
Jan 15, 2021
| @@ -0,0 +1,3 @@ | |||
| declare const _default: typeof import('./lib/cjs/puppeteer/node').default; | |||
|
|
|||
| export default _default; | |||
There was a problem hiding this comment.
a cjs entry should have an export = definition.
|
also, ideally compatibility with the existing |
|
For those wondering how to load (like me) the current d.ts until it's released: tsconfig.json
{
"paths": {
"puppeteer-core": [
"node_modules/puppeteer-core/lib/esm/puppeteer/node"
]
}
} |
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 PR replaces #6289 and
instead puts all TS typedefs into one folder, lib/typings, which we then
can point TS to via the
typesfield inpackage.json.The previous PR (#6289) tried
to merge all our typedefs into one big file via API Extractor, but this
isn't really necessary. We can instead put all the individual
.d.tsfiles into one folder, and TS will still understand them and provide a
good experience to the developer.
One important consideration is that you could consider this a breaking
change. I'm not sure how likely it is, but it could cause problems for:
its exports as
anymay now start having legitimate type failures.@types/puppeteerpackage may have issuesif they now swap to use this one and the types aren't quite aligned.
In addition, once we do ship a release with this change in, it will mean
that we have to treat any changes to any type definitions as
release-note-worthy, and any breaking changes to type definitions will
need to be treated as breaking code changes (nearly always a breaking
type def means a breaking change anyway), but it's worth considering
that once we expose these typedefs we should treat them as first class
citizens of the project just like we would with the actual source code.