-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Enforce type checking on babel-{cli,node}
#14765
Conversation
packages/babel-cli/src/babel/file.ts
Outdated
@@ -1,5 +1,7 @@ | |||
import convertSourceMap from "convert-source-map"; | |||
import { AnyMap, encodedMap } from "@jridgewell/trace-mapping"; | |||
import type { Section } from "@jridgewell/trace-mapping/dist/types/types"; |
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.
I am not comfortable with the undeclared package sub imports. @jridgewell Can you make the Section
type public?
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.
Sure. Does this serve the need for now?
import type { SectionedSourceMap } from '@jridgewell/trace-mapping';
type Section = SectionedSourceMap['sections'][0]
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.
Yes, Thanks!
@@ -22,7 +23,7 @@ export function enable({ enableGlobbing }: { enableGlobbing: boolean }) { | |||
stabilityThreshold: 50, | |||
pollInterval: 10, | |||
}, | |||
}); | |||
} as WatchOptions); |
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.
Does const options: WatchOptions = { ... }
work? Casting WatchOptions
will mute any chokidar config errors.
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.
Yes it can, but it should be able to type check even with as WatchOptions
.
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.
IIUC using as
is basically a cast, it will fail if the types are incompatible but it won't point out things like missing required properties.
cli
and node
babel-{cli,node}
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52559/ |
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.
Approving but I would like to see #14765 (comment) resolved
filter?: (filename: string, index: number, dir: string) => boolean | ||
): string[]; | ||
export = read; | ||
} |
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.
As a follow-up, could you open a PR to update https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/fs-readdir-recursive/index.d.ts so that then we can use @types/fs-readdir-recursive
?
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.
DefinitelyTyped/DefinitelyTyped#61344
Finish. I think we might be able to remove this package at some point.
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
102d45f
to
938f922
Compare
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Fixes #1, Fixes #2
These two packages seem to be left out.
I use a couple of
@ts-expect-error
because the imported packages have no or incorrect type definitions, but that doesn't matter because only one method belonging to them is used.