Skip to content
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

feat(errors): validate preset when filename is absent #10181

Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 9, 2019

Closes #10154

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

Validate preset before buildPresetChain, when it is the last chance we can get the UnloadedDescriptor and include preset name in error message. We may add more business logic inside validatePreset to print preset-specific error message.

I have also considered the following alternative designs:

  1. Pass down descriptor name to matchPattern.
    Pro: The missing filename errors are still thrown in matchPattern
    Cons: Introduce unnecessary signature complexity as this is not our primary execution path

  2. Add descriptor name to ConfigContext
    Pro: The missing filename errors are still thrown in matchPattern
    Cons: ConfigContext has to be mutated or cloned when running buildPresetChain. The ConfigContext is widely used across config chain building but descriptor name is only used to print this error.

  3. tryCatch buildPresetChain
    Pros: Intuitive and fewer execution path
    Cons: Any error from buildPresetChain has to be matched once before they are re-thrown.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 9, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11468/

if (options.test || options.include || options.exclude) {
throw new Error(
`Preset ${descriptor.name ||
""} requires filename, but it was not passed.`.replace(/\s{2}/, " "),
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to link directly to the option:

Preset ${name} requires a filename be set. See https://babeljs.io/docs/en/options#filename.

Or even:

Preset ${name} requires a filename be set. For example, if you are calling Babel directly:

babel.transform(code, { filename: 'file.js', presets: [${name || '/* your preset */'}] });

See https://babeljs.io/docs/en/options#filename for more information.

(And, as you mentioned, improve this later with user- and preset-specific messages)

Copy link
Contributor Author

@JLHwung JLHwung Jul 9, 2019

Choose a reason for hiding this comment

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

That delivers way better dev UX. Addressed in cf0f27a.

@existentialism
Copy link
Member

existentialism commented Jul 9, 2019

Should we also update the language on the options page (https://babeljs.io/docs/en/options#filename)?

Maybe simply changing:

The filename is exposed to plugins. Some plugins may require the presence of the filename.

to:

Some presets and plugins may require the presence of the filename.

Or, since we get the question a bunch, call out preset-typescript's behavior directly here?

(We can continue discussion on the website repo after this lands)

): void => {
if (!context.filename) {
const { options } = preset;
if (options.test || options.include || options.exclude) {
Copy link
Member

Choose a reason for hiding this comment

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

We should also check if the overrides option contains a block which has one of these three options (like in the TS preset).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed in 056e197.

@nicolo-ribaudo nicolo-ribaudo added area: errors PR: New Feature 🚀 A type of pull request used for our changelog categories area: typescript pkg: core labels Jul 9, 2019
JLHwung added a commit to JLHwung/babel that referenced this pull request Jul 9, 2019
@JLHwung JLHwung force-pushed the check-filename-on-building-preset branch from ffc6a72 to cf0f27a Compare July 10, 2019 01:22
JLHwung added a commit to JLHwung/babel that referenced this pull request Jul 10, 2019
@nicolo-ribaudo
Copy link
Member

(Note: I labeled this as "New feature" but I I think that this PR can be merged in a patch version)

@nicolo-ribaudo nicolo-ribaudo added this to the v7.6.0 milestone Aug 14, 2019
: "/* your preset */";
throw new Error(
[
`Preset ${formattedPresetName} requires a filename be set.`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read again and feel like

Preset ${formattedPresetName} requires a filename to be set when babel is called directly,
`\`\`\``,
`babel.transform(code, { filename: 'file.ts', presets: [${formattedPresetName}] });`,
`\`\`\``

And we may can change file.js to file.ts since this is the popular path.

Copy link
Member

Choose a reason for hiding this comment

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

And we may can change file.js to file.ts since this is the popular path

Good idea

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019
@JLHwung JLHwung deleted the check-filename-on-building-preset branch December 15, 2020 15:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: errors area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: core PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@babel/standalone transform failed when using preset-typescript
4 participants