polish: throw human-friendly error when item-option pair is incorrectly unwrapped

[JLHwung]

Q	A
Tests Added + Pass?	Yes
License	MIT

I want to focus on the scenario I want to help instead of giving too much technical details of this PR. Anyway the approach is straightforward.

When you begin to use babel, the config may be as simple as

{
  "presets": ["@babel/env"]
}

It works out of box. It is great.

After you have learned more about babel, you may begin to add options to preset-env, let's say you try

{
  "presets": ["@babel/env", { "useBuiltIns": true }]
}

It looks quite intuitive, right? But unfortunately it will throw

[BABEL] unknown: Unknown option: .useBuiltIns. Check out https://babeljs.io/docs/en/babel-core/#options for more information about options.

The error message is technically correct. But this message is tremendously confusing even if you are a babel veteran because .useBuiltIns is an option for preset-env, and especially after you have double check you didn't wrote it as useBuiltins. But why babel throws unknown options for .useBuiltIns? Well when I was learning to configure babel, it takes me quite a while before I realize that I should add a [] to wrap the preset name and its options

presets: [["@babel/env", { "useBuiltIns": true }]]

In this PR we detect this error pattern and give a human friendly error message.

[nicolo-ribaudo]

❤️ ❤️ ❤️ ❤️

The failing test is still a false positive

[JLHwung]

@nicolo-ribaudo Yeah, I realize that validate does not support plugin type, although it says it accepts any OptionsSource-kind. To minimize the change of this PR, I have to add plugin support to validate.

Never mind, it turns out I have confused between PluginObject and PluginOptions.

[loganfsmyth]

I think the location for this check is not idea. Would you be open to doing this at

• When the plugin is instantiated:

  babel/packages/babel-core/src/config/full.js

  Line 75 in b64cb9a

  acc.push(loadPluginDescriptor(descriptor, context));

• Where the preset is instantiated:

  babel/packages/babel-core/src/config/full.js

  Line 82 in b64cb9a

  preset: loadPresetDescriptor(descriptor, context),

You should be able to catch the error there and see that it was a plain object immediately after a successfully-loaded plugin/preset. It also seems like it could be a lot nicer to append text to the error's message saying something like "maybe you forgot an extra pair of square brackets" instead of throwing a while separate error, and you could do it for the more general case withing special-casing the single-preset + plain object case.

[JLHwung]

@loganfsmyth Thanks for your suggestion. I am open to move the check to the reduce step, but I am hesitant to generalize this check to a very board case.

There are various cases that one can generalize this check into.

1. When the last config item is valid and this config item is non-valid plain object

This is the most generalized case and it almost replaces the original validation error because any error in the non-first config item will result to this error. I am opposed to this approach as it introduces too much noise for advanced users.

2. When the last config item is a valid resolve request

That means we will print this message on

{
  "presets": [
    [
      "@babel/preset-env",
      {
        "targets": {
          "esmodules": true
        }
      },
     "@babel/preset-typescript"
    ]
  ]
}

Well in this case we can print this message as a better guess than 1). I can implement this requirement if we can reach consensus here.

It also seems like it could be a lot nicer to append text to the error's message saying something like "maybe you forgot an extra pair of square brackets" instead of throwing a while separate error.

I still favour the separate error message because it is more focused. Let's do a comparison:

This is the error message while appending "maybe you forgot an extra pair of square brackets"

[BABEL] unknown: Unknown option: .useBuiltIns. Check out
https://babeljs.io/docs/en/babel-core/#options for more information about options.
Maybe you forgot an extra pair of square brackets (`[]`).

Unless we improve the error message logic, the leading double unknown words will be really daunting to users: it seems like the config is terribly wrong. And what's worse, the url does not points to the document of the presets, although all of them are technically correct.

While this is the tuned message

.plugin[1] is not a valid plugin. Maybe you meant to use
"plugin": [
  ["@babel/transform-react-jsx", {"useSpread":true}]
]
To be a valid plugin, its name and options should be wrapped in a pair of brackets

It emphasizes the possible copy-paste ready solution over the technical errors. I can prepend [BABEL] to the message so it is more visually consistent.

[loganfsmyth]

This is the most generalized case and it almost replaces the original validation error because any error in the non-first config item will result to this error. I am opposed to this approach as it introduces too much noise for advanced users.

Part of what I'm saying is that I don't think we should replace the error at all. Just like we do with

babel/packages/babel-core/src/config/files/plugins.js

Line 135 in e85faec

e.message += `\n- Did you accidentally pass a ${oppositeType} as a ${type}?`;

for instance, we could append to the "unknown property" validation error saying It looks like this object might be options for a plugin/preset, did you mean to wrap this in a set of [] to apply these options?

To help clarify things, something like

  const plugins = config.plugins.reduce((acc, descriptor, i) => {
    if (descriptor.options !== false) {
      try {
        acc.push(loadPluginDescriptor(descriptor, context));
      } catch (err) {
        if (typeof descriptor.value === "object" && i > 0 && config.plugins[i].options === undefined) {
          err.message += " It looks like this object might be options for the plugin/preset before it. When applying options, the options need to be grouped with the plugin/preset as a single entry in the overall plugin/preset list. Did you forget to group them with `[]`?";
        }
        throw err;
      }
    }
    return acc;
  }, []);

I get your concerns over error message verbosity though, and I admit I don't feel that strongly. The main reason I comment on this is because the placement of the logic had surprised me. For your case, we could instead do

  const plugins = config.plugins.reduce((acc, descriptor, i) => {
    if (descriptor.options !== false) {
      try {
        acc.push(loadPluginDescriptor(descriptor, context));
      } catch (err) {
        if (
          config.plugins.length === 2 && 
          config.plugins[0].file &&
          config.plugins[0].options === undefined &&
          i === 1 && 
          typeof descriptor.value === "object"
        ) {
            throw new Error(
              `.plugins[1] is not a valid plugin. Maybe you meant to use\n` +
              `"plugin": [\n  ["${config.plugins[i].file.request}", ${JSON.stringify(descriptor.value)}]\n]\n` +
              `To be a valid plugin, its name and options should be wrapped in a pair of brackets`,
               );
        }
        throw err;
      }
    }
    return acc;
  }, []);

and I think it would be clearer and avoid repeating the validation logic.

[nicolo-ribaudo]

If we are not checking if config.plugins is 2, we should also have a test with more plugins.

[JLHwung]

Can do.

[nicolo-ribaudo]

I think @loganfsmyth's concerns have been addressed.