Better error messaging when preset options are given without a corresponding preset #4685

kaicataldo · 2016-10-06T03:25:24Z

Q	A
Bug fix?	no
Breaking change?	no
New feature?	yes, I think?
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets	#4677
License	MIT
Doc PR	reference to the documentation PR, if any

Checking for the presence of the presets array in mergeOptions and then checking the elements in that array seems like the best way to do this to me. The other place I looked at putting this check was in resolvePresets, but that didn't seem as reliable as this (lots more cases to account for).

~~Let me know if you had something else or a better solution in mind!~~

Edit: See comment below

hzoo · 2016-10-06T03:32:57Z

You can use the PR template to ref #4677 (if this is the one you are talking about)

kaicataldo · 2016-10-06T03:34:39Z

Sorry, used hub and missed the template. Will fill it out!

codecov-io · 2016-10-06T04:07:16Z

Current coverage is 88.83% (diff: 100%)

Merging #4685 into master will increase coverage by <.01%

@@             master      #4685   diff @@
==========================================
  Files           195        195          
  Lines         13794      13795     +1   
  Methods        1427       1427          
  Messages          0          0          
  Branches       3176       3176          
==========================================
+ Hits          12254      12255     +1   
  Misses         1540       1540          
  Partials          0          0

Powered by Codecov. Last update 760bbd6...3df4952

hzoo · 2016-10-07T00:02:58Z

packages/babel-core/src/transformation/file/options/option-manager.js


        if (removed[key]) {
          this.log.error(`Using removed Babel 5 option: ${alias}.${key} - ${removed[key].message}`, ReferenceError);
+        } else if (opts.presets && Array.isArray(opts.presets)) {
+          for (let i = 0; i < opts.presets.length; i++) {
+            let preset = opts.presets[i];


if you want you can do a for of here: for (preset of opts.presets) {

Happy to change if that's the preference :)

hzoo · 2016-10-07T00:05:29Z

packages/babel-core/src/transformation/file/options/option-manager.js

+          for (let i = 0; i < opts.presets.length; i++) {
+            let preset = opts.presets[i];
+            if (typeof preset === "object" && preset !== null && !Array.isArray(preset)) {
+              this.log.error(`Found configuration options ${JSON.stringify(opts)} without corresponding preset. ${presetConfigInfo}`);


Should we give an example here (it would be kinda long) or is the link fine?

{ "presets": [ ["presetHere", { presetOptions: "here"}] // wrap preset/options in an array ] }

Seemed like a lot to log, which is why I put the link in. Can add that in if you think it's better though!

Yeah we can go with the link too - dono what others think. I tend to like being verbose

hzoo

no blockers just comments

hzoo · 2016-10-08T15:32:13Z

So i'm testing this myself in a new project and it doesn't look like it's correct? The tests are correct but it's not the same options

I tried doing

    "presets": [
      "stage-2", { "loose2": false }
    ]

and where you are doing opts.presets I get undefined since opts is { loose2: false}

kaicataldo · 2016-10-08T18:17:05Z

Weird, I thought my tests covered that? Looking into it

kaicataldo · 2016-10-08T18:39:11Z

The tests I added don't actually fail now. They were definitely failing before, because I remember having to add a "real" fixture preset for one of them (was erroring that the non-existent preset didn't accept options).

hzoo · 2016-10-08T18:40:38Z

What I did was a npm link in babel/packages/babel-cli and then trying the error to see the message

kaicataldo · 2016-10-08T19:00:39Z

Oh, I figured it out - assert.throws() only throws when the second argument is a regex, and I was giving it a string

kaicataldo · 2016-10-08T19:20:31Z

Now that I have reliable tests, this test is failing (correctly) with my changes: https://github.com/babel/babel/blob/master/packages/babel-core/test/api.js#L191

It looks like objects are allowed sometimes - when is that? Is it only if it has the plugins key? I don't see any examples of this use case in the docs, so not sure what is allowed/what isn't.

For example, one of the test errors is showing that this object is in the presets array (is this right?):

{"plugins":[[null,{"loose":false,"spec":false}],null,null,[null,{"spec":false}],null,[null,{"loose":false}],null,null,null,[null,{"loose":false}],[null,{"loose":false}],null,null,null,[null,{"loose":false}],null,[null,{"loose":false}],null,null,[null,{"loose":false}],[null,{"async":false,"asyncGenerators":false}]]}

Thanks for the guidance on this one!

kaicataldo · 2016-10-09T06:14:56Z

Is this maybe the resulting config created by presets? Hmmm...If so, this is trickier than I first thought, because what we actually get back here is the mutated config option rather than the original configuration written by the user. This might be the wrong place to throw this error then, since at this point we have no knowledge of what the original config was.

hzoo · 2016-10-10T13:59:43Z

I think the issue is that objects are allowed because that is what the preset ends up being (an object with certain keys like plugins/presets). You can totally define a preset inline as an object if you are using the api (not a babelrc which is just json)

…ponding preset

kaicataldo · 2016-10-11T01:45:28Z

After my investigation and our discussion, here's the simpler and more reliable solution of just giving more helpful information in the logged error.

danez · 2016-10-11T14:56:59Z

Cool thanks

…ponding preset (babel#4685)

kaicataldo force-pushed the fixes4677 branch 2 times, most recently from 3a964e6 to 7672129 Compare October 6, 2016 03:28

kaicataldo force-pushed the fixes4677 branch 2 times, most recently from 42c753d to 2c0a21b Compare October 6, 2016 04:07

kaicataldo force-pushed the fixes4677 branch 3 times, most recently from 92d41c3 to b386e3a Compare October 6, 2016 04:41

hzoo reviewed Oct 7, 2016

View reviewed changes

hzoo added the PR: Polish 💅 A type of pull request used for our changelog categories label Oct 7, 2016

hzoo approved these changes Oct 7, 2016

View reviewed changes

kaicataldo force-pushed the fixes4677 branch 3 times, most recently from 3c693aa to 8fdfb5e Compare October 7, 2016 00:46

kaicataldo force-pushed the fixes4677 branch from 5fc3585 to aa982da Compare October 8, 2016 19:32

kaicataldo changed the title ~~Better error messaging when preset options are given without a corresponding preset~~ WIP: Better error messaging when preset options are given without a corresponding preset Oct 8, 2016

kaicataldo changed the title ~~WIP: Better error messaging when preset options are given without a corresponding preset~~ Better error messaging when preset options are given without a corresponding preset Oct 11, 2016

kaicataldo force-pushed the fixes4677 branch 2 times, most recently from 0830d57 to 9b05c74 Compare October 11, 2016 01:30

Better error messaging when preset options are given without a corres…

3df4952

…ponding preset

kaicataldo force-pushed the fixes4677 branch from 9b05c74 to 3df4952 Compare October 11, 2016 01:37

danez merged commit 27ee74e into babel:master Oct 11, 2016

kaicataldo deleted the fixes4677 branch October 11, 2016 14:59

panagosg7 pushed a commit to panagosg7/babel that referenced this pull request Jan 17, 2017

Better error messaging when preset options are given without a corres…

59a25f2

…ponding preset (babel#4685)

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error messaging when preset options are given without a corresponding preset #4685

Better error messaging when preset options are given without a corresponding preset #4685

kaicataldo commented Oct 6, 2016 •

edited

hzoo commented Oct 6, 2016 •

edited

kaicataldo commented Oct 6, 2016

codecov-io commented Oct 6, 2016 •

edited

hzoo Oct 7, 2016

kaicataldo Oct 7, 2016

hzoo Oct 7, 2016

kaicataldo Oct 7, 2016

hzoo Oct 7, 2016

hzoo left a comment

hzoo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016

hzoo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016 •

edited

kaicataldo commented Oct 9, 2016 •

edited

hzoo commented Oct 10, 2016 •

edited

kaicataldo commented Oct 11, 2016 •

edited

danez commented Oct 11, 2016

Better error messaging when preset options are given without a corresponding preset #4685

Better error messaging when preset options are given without a corresponding preset #4685

Conversation

kaicataldo commented Oct 6, 2016 • edited

hzoo commented Oct 6, 2016 • edited

kaicataldo commented Oct 6, 2016

codecov-io commented Oct 6, 2016 • edited

Current coverage is 88.83% (diff: 100%)

hzoo Oct 7, 2016

Choose a reason for hiding this comment

kaicataldo Oct 7, 2016

Choose a reason for hiding this comment

hzoo Oct 7, 2016

Choose a reason for hiding this comment

kaicataldo Oct 7, 2016

Choose a reason for hiding this comment

hzoo Oct 7, 2016

Choose a reason for hiding this comment

hzoo left a comment

Choose a reason for hiding this comment

hzoo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016

hzoo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016

kaicataldo commented Oct 8, 2016 • edited

kaicataldo commented Oct 9, 2016 • edited

hzoo commented Oct 10, 2016 • edited

kaicataldo commented Oct 11, 2016 • edited

danez commented Oct 11, 2016

kaicataldo commented Oct 6, 2016 •

edited

hzoo commented Oct 6, 2016 •

edited

codecov-io commented Oct 6, 2016 •

edited

kaicataldo commented Oct 8, 2016 •

edited

kaicataldo commented Oct 9, 2016 •

edited

hzoo commented Oct 10, 2016 •

edited

kaicataldo commented Oct 11, 2016 •

edited