-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Improve @babel/core types
#17404
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
Improve @babel/core types
#17404
Conversation
| | ((api: PluginAPI, options?: object, dirname?: string) => PluginObject); | ||
| export type PluginItem = | ||
| | ConfigItem<PluginAPI> | ||
| | Plugin |
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.
This is a change, Plugin is our internal class and should not be accepted.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60052 |
|
|
||
| for (let i = 0; i < rawPresets.length; i++) { | ||
| const descriptor = rawPresets[i]; | ||
| // @ts-expect-error TODO: disallow false |
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.
What do you think?
Currently Babel accepts ["plugin", false] to disable a plugin, is there any reason for this?
Currently some private properties are also exposed, I will open a new PR. |
|
commit: |
827283d to
94299e4
Compare
| const options: BaseOptions = { | ||
| ...opts, | ||
|
|
||
| parserOpts: { | ||
| sourceType: | ||
| path.extname(filenameRelative) === ".mjs" ? "module" : sourceType, | ||
|
|
||
| // @ts-expect-error FIXME: should be sourceFilename | ||
| sourceFileName: filename, |
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.
This is a bug, but requires a more complex fix, which I'll include in a separate PR.
0765993 to
e139554
Compare
| filepath: string; | ||
| dirname: string; | ||
| options: ValidatedOptions; | ||
| options: InputOptions; |
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.
This isn't ideal, but I haven't figured out a better solution yet.
JLHwung
left a comment
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 agree that we should probably stop exporting ValidatedOptions, but we still have to export InputOptions and NormalizedOptions, since the latter can be accessed from the return result of the loadPartialConfig API via the .options property.
| plugins?: Plugin[]; | ||
| }; | ||
|
|
||
| export type NormalizedOptions = { |
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.
Could you add JSDoc description comments on InputOptions, ValidatedOptions and NormalizedOptions?
Based on the shape it seems to me that ValidatedOptions is the option after the plugins / presets are resolved, and NormalizedOptions is the option after targets are resolved. But the their namings currently do not really indicate their lifecycles.
And among them only InputOptions are exported to users right?
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 did some refactoring, and it looks much better now.
- Renamed
ValidatedOptionstoResolvedOptions - Better defined
NormalizedOptionsso it can now be assigned toInputOptions.
I hesitated about exporting ResolvedOptions; obviously, we don't want users to read or write it, but as a public function return value, exporting it would make caching easier for users.
NormalizedOptions has been exported.
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.
Better defined NormalizedOptions so it can now be assigned to InputOptions.
I don't think it is safe to assign NormalizedOptions to an InputOptions, because:
InputOptions is the options for the API users, we will then provide defaults, enforces validations and resolve plugins/ presets so that it became a ResolvedOptions. Then we resolve targets and it became NormalizedOptions. At this point, the targets are readonly, plugins are resolved, it should not be cast to an InputOptions -- since we are not going to resolve targets / plugins again.
If there are typing errors for doing so, maybe those methods actually expects NormalizedOptions rather than InputOptions and we should then update those methods.
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.
From what I understand, InputOptions will first become NormalizedOptions and then become ResolvedOptions?
We have an existing usage in eslint-parser of passing loadPartialConfigSync().options into parseSync which seems to be acceptable.
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.
Okay. If that's the case, that means ResolvedOptions must be made a subset type of InputOptions, so we should export the internal resolved plugin type, since it is indeed accepted by our API.
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.
Could you explain more, thanks!
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.
Sorry, I don't understand what you mean. The API accepts
NormalizedOptionsbut notResolvedOptions.
Sorry for the late reply. Let me try to elaborate a bit.
The parseSync accepts InputOptions.
babel/packages/babel-core/src/parse.ts
Line 26 in 900e5d1
| opts: InputOptions | undefined | null, |
Now if parseSync also supports the return result of loadPartialConfigSync().options, then the NormalizedOptions, returned from loadPartialConfigSync().options,
| options: NormalizedOptions; |
should be automatically also a valid InputOptions. This implies that ConfigItem<PluginAPI>[] should be accepted in InputOptions, because of the usage in eslint-parser. And Plugin should be broaden to include ConfigItem<PluginAPI>.
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.
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.
And
Pluginshould be broaden to include ConfigItem.
Sorry for any confusion here. To clarify, Plugin is not an typo of PluginItem. I refer to the Plugin type defined here:
| export default class Plugin { |
nested within the ResolvedConfig in packages/babel-core/src/config/full.ts.
Since ResolvedOptions is allowed as an InputOption because of the usage in eslint-parser, we should make typing adjustments to InputOptions such that any ResolvedOptions will also satisfy InputOptions, but not the opposite. Or we provide overload signatures to the API for the ResolvedOptions support.
The goal here is that end users should not cast typings if they only use Babel API, as long as the API accepts such inputs.
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 checked and it seems that eslint-parser does not use ResolvedOptions, although there is a function called validateResolvedConfig, it actually returns still InputOptions | NormalizedOptions.
| export type { | ||
| CallerMetadata, | ||
| NormalizedOptions, | ||
| ValidatedOptions as PresetObject, |
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.
Is this type export still used now that we have created PresetObject type in the root index.ts?
| }); | ||
|
|
||
| const options: NormalizedOptions = { | ||
| const options: InputOptions = { |
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.
We only resolve the targets here, isn't the options here the NormalizedOptions? That is, the targets should be considered read-only because it won't be resolved again if they are somehow modified by plugins.
f626409 to
900e5d1
Compare
| PluginItem, | ||
| } from "./config/validation/options.ts"; | ||
| export { loadOptionsSync }; | ||
| export type { PluginItem }; |
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.
Thanks for this, currently for Jest I had to use type PluginItem = NonNullable<BabelOptions['plugins']>[number]; instead 😅
JLHwung
left a comment
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.
Aside: I also noticed that there is another normalizeOptions routine:
| function normalizeOptions(opts: ValidatedOptions): ValidatedOptions { |
I think this routine can be renamed to createConfigChainOptions and we should create a new ConfigChainOptions type for it, it is also a subset of InputOptions but with quite a few properties removed. This should catch errors if we try to access those properties from the config chain options.
| export default function normalizeOptions( | ||
| config: ResolvedConfig, | ||
| ): NormalizedOptions { | ||
| ): ResolvedOptions { |
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.
The type NormalizedOptions is named after this routine, it would be very confusing if this routine returns ResolvedOptions. Maybe we can rename it as buildResolvedConfigOptions or something.
| plugins?: Plugin[]; | ||
| }; | ||
|
|
||
| export type NormalizedOptions = { |
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.
Sorry, I don't understand what you mean. The API accepts
NormalizedOptionsbut notResolvedOptions.
Sorry for the late reply. Let me try to elaborate a bit.
The parseSync accepts InputOptions.
babel/packages/babel-core/src/parse.ts
Line 26 in 900e5d1
| opts: InputOptions | undefined | null, |
Now if parseSync also supports the return result of loadPartialConfigSync().options, then the NormalizedOptions, returned from loadPartialConfigSync().options,
| options: NormalizedOptions; |
should be automatically also a valid InputOptions. This implies that ConfigItem<PluginAPI>[] should be accepted in InputOptions, because of the usage in eslint-parser. And Plugin should be broaden to include ConfigItem<PluginAPI>.
ea0f6bb to
8f32ea5
Compare
Co-authored-by: Huáng Jùnliàng <[email protected]>

Fixes #1, Fixes #2The main change is to distinguish
InputOptionsfromValidatedOptions, because they have two different fields.To be honest, I don't want to expose some internal types such as
Pluginor evenValidatedOptionsdetails.But I didn't think of a good way.