Skip to content

Conversation

@liuxingbaoyu
Copy link
Member

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

The main change is to distinguish InputOptions from ValidatedOptions, because they have two different fields.

To be honest, I don't want to expose some internal types such as Plugin or even ValidatedOptions details.
But I didn't think of a good way.

| ((api: PluginAPI, options?: object, dirname?: string) => PluginObject);
export type PluginItem =
| ConfigItem<PluginAPI>
| Plugin
Copy link
Member Author

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.

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 27, 2025

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
Copy link
Member Author

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?

@liuxingbaoyu
Copy link
Member Author

To be honest, I don't want to expose some internal types such as Plugin or even ValidatedOptions details.
But I didn't think of a good way.

Currently some private properties are also exposed, I will open a new PR.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jul 28, 2025

Open in StackBlitz

commit: 030d0c1

Comment on lines 28 to 39
const options: BaseOptions = {
...opts,

parserOpts: {
sourceType:
path.extname(filenameRelative) === ".mjs" ? "module" : sourceType,

// @ts-expect-error FIXME: should be sourceFilename
sourceFileName: filename,
Copy link
Member Author

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.

filepath: string;
dirname: string;
options: ValidatedOptions;
options: InputOptions;
Copy link
Member Author

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.

Copy link
Contributor

@JLHwung JLHwung left a 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 = {
Copy link
Contributor

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?

Copy link
Member Author

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.

  1. Renamed ValidatedOptions to ResolvedOptions
  2. Better defined NormalizedOptions so it can now be assigned to InputOptions.

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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!

Copy link
Contributor

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 NormalizedOptions but not ResolvedOptions.

Sorry for the late reply. Let me try to elaborate a bit.

The parseSync accepts InputOptions.

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>.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Thanks for the detailed explanation.
It looks like PluginItem already contains ConfigItem<PluginAPI>?

Copy link
Contributor

@JLHwung JLHwung Sep 23, 2025

Choose a reason for hiding this comment

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

And Plugin should 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.

Copy link
Member Author

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,
Copy link
Contributor

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 = {
Copy link
Contributor

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.

@liuxingbaoyu liuxingbaoyu added pkg: core PR: Polish (next major) 💅 A type of pull request used for our changelog categories for next major release labels Sep 4, 2025
@liuxingbaoyu liuxingbaoyu requested a review from JLHwung September 9, 2025 17:33
PluginItem,
} from "./config/validation/options.ts";
export { loadOptionsSync };
export type { PluginItem };
Copy link
Member

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 😅

Copy link
Contributor

@JLHwung JLHwung left a 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 {
Copy link
Contributor

@JLHwung JLHwung Sep 22, 2025

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 = {
Copy link
Contributor

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 NormalizedOptions but not ResolvedOptions.

Sorry for the late reply. Let me try to elaborate a bit.

The parseSync accepts InputOptions.

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>.

This was referenced Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg: core PR: Polish (next major) 💅 A type of pull request used for our changelog categories for next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants