Skip to content

Add TypeScript definitions for public packages #6484

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

Merged
merged 6 commits into from
Jun 26, 2021
Merged

Add TypeScript definitions for public packages #6484

merged 6 commits into from
Jun 26, 2021

Conversation

devongovett
Copy link
Member

Closes #5297. Fixes #3912.

This adds TypeScript definition files for public packages including @parcel/core, @parcel/types, and @parcel/plugin along with all dependencies. This will make it easier for people to develop plugins and use the Parcel API in TypeScript (or even just for autocomplete in VSCode).

I tried to use flow-to-ts on these packages, and then run tsc to generate .d.ts files, but ran into some issues because the TS type system and Flow type system work significantly differently and I was getting lots of errors when TS checked the converted code.

Instead, I opted for a hybrid approach. I used flow-to-ts to convert the type-only files (e.g. @parcel/types), but wrote some hand written types for some packages containing code. This way, the bulk of changes will be handled automatically and kept in sync with the Flow types, but we don't need to worry about passing two type systems on all source code.

@height
Copy link

height bot commented Jun 20, 2021

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@@ -1,5 +1,4 @@
// @flow strict-local
import type {FilePath} from '@parcel/types';
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoiding circular dependency... Perhaps we should consider whether the diagnostic types should move into the core types package and only the implementation should stay in this package?

@@ -195,11 +195,11 @@ export class MemoryFS implements FileSystem {
}

// eslint-disable-next-line require-await
async readFile(filePath: FilePath, encoding?: buffer$Encoding): Promise<any> {
async readFile(filePath: FilePath, encoding?: Encoding): Promise<any> {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can't rely on these global flow types

Validator as ValidatorOpts,
} from '@parcel/types';

export declare class Transformer<T> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Did this file manually because flow allows the type parameter on the constructor rather than the class but TS doesn't. Wanted to avoid the type param on the class in Flow otherwise you have to explicitly type the config on every plugin rather than it being inferred. This is because flow doesn't infer the types of exports in type first mode and forces you to type it explicitly (which is really annoying).

@@ -875,6 +878,36 @@ export type TransformerResult = {|

export type Async<T> = T | Promise<T>;

export interface PluginLogger {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this type here to avoid another circular dependency. Plus, better to expose an interface to plugins rather than an implementation.

@parcel-benchmark
Copy link

parcel-benchmark commented Jun 20, 2021

Benchmark Results

Kitchen Sink 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description Time Difference
Cold 10.56s +112.00ms
Cached 458.00ms -15.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description Time Difference
Cold 7.85s -86.00ms
Cached 397.00ms -30.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/Three.js 579.69kb +0.00b 5.77s -404.00ms 🚀

Click here to view a detailed benchmark overview.

@zxbodya
Copy link

zxbodya commented Jun 22, 2021

Hi,
I am building a tool for converting flow code into TypeScript - flowts.

And, I happened to also experiment with it on parcel codebase, on which there is pretty good progress - rebased it on current latest parcel version:

https://github.com/zxbodya/parcel/tree/ts

If one day, you decide to migrate parcel to TypeScript - that might be a good starting point 😄

There is still some type errors to be fixed, and also to review fixes I did manually after converting it to typescript, but generally it already should be in a pretty good shape, and can at least - can be used for generating type definitions.

Drafted scripts to bundle d.ts files for publishing:

It should be usable, only thing - probably some type dependencies to be added in package.json files before merging it.

@devongovett devongovett merged commit f2a5588 into v2 Jun 26, 2021
@devongovett devongovett deleted the ts-types branch June 26, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Parcel 2] Typescript Definitions
4 participants