Point out when a plugin needs to be enabled for encountered syntax (#169) #178

motiz88 · 2016-10-14T22:09:20Z

Ref #169
This mainly replaces the if (!this.hasPlugin("foo")) this.unexpected(); idiom with a new parser helper method, expectPlugin.

Right now the error is worded like this: This experimental syntax requires the foo parser plugin. Check out babel-plugin-syntax-foo, babel-plugin-transform-foo or the parserOpts.foo Babel option. Open for different suggestions of course.

Another idiom we may want to change is where we currently check for things like this.hasPlugin("asyncGenerators") && this.eat(tt.star). Does it make sense to flip the order in some places? Match the special token (here *) without advancing, then assert that we have the plugin, then advance. In cases where we can be sure of the user's intent this looks like a usability win.

…abel#169)

motiz88 · 2016-10-15T06:13:25Z

codecov-io · 2016-10-15T14:30:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@57aacea). Click here to learn what that means.
The diff coverage is 77.77%.

@@            Coverage Diff            @@
##             master     #178   +/-   ##
=========================================
  Coverage          ?   96.28%           
=========================================
  Files             ?       20           
  Lines             ?     3205           
  Branches          ?      836           
=========================================
  Hits              ?     3086           
  Misses            ?       61           
  Partials          ?       58

Impacted Files	Coverage Δ
src/parser/statement.js	`97.1% <100%> (ø)`
src/plugins/directory.js	`100% <100%> (ø)`
src/parser/expression.js	`92.81% <52%> (ø)`
src/parser/util.js	`88.63% <84.61%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57aacea...880fbdd. Read the comment docs.

# Conflicts: # test/fixtures/experimental/no-async-generators/error-without-plugin/options.json

motiz88 · 2016-10-16T13:51:22Z

#182 will probably take care of most of the gap in code coverage

motiz88 · 2016-10-17T11:12:38Z

Thanks @danez! Do you have an opinion on this.hasPlugin("asyncGenerators") && this.eat(tt.star) ➡️ this.match(tt.star) && this.expectPlugin("asyncGenerators"), this.next() etc?

danez · 2016-10-17T11:29:13Z

I think this would also be the only way to do it in this cases right? Because otherwise doing this.eat() && this.expectPlugin()would change the location of the error.
So yes that sounds good

motiz88 · 2016-10-17T12:56:58Z

Yeah. The thing to watch out for, if we eagerly match() && expectPlugin(), is cases where there is an alternative correct parse we can't rule out yet. Async generators are not generally such a case - after async function, if we see * it's either an async generator (with the plugin) or an error (under any other parser config), so it's a safe bet the user meant it as an async generator.

danez · 2016-10-17T12:58:47Z

True, so probably we need to decide for each occurrence separately.

motiz88 · 2016-10-17T13:01:27Z

OK, I'll just add some of the easy cases here real quick.

danez · 2016-11-11T09:02:40Z

src/parser/expression.js

-        this.state.inFunction = oldInFunction;
-        this.state.labels = oldLabels;
-        return this.finishNode(node, "DoExpression");
-      }


We had a fall-through here before without doExpression, but that looks like a mistake anyway. Why should do fall-through to regexp?

danez · 2016-11-11T09:05:49Z

src/parser/expression.js

+      this.expectPlugin("functionSent");
+    } else if (!this.hasPlugin("functionSent")) {
+      // They didn't actually say `function.sent`, just `function.`, so a simple error would be less confusing.
+      this.unexpected();


Uncovered. We could test this I think easily.

danez · 2016-11-11T09:10:12Z

src/parser/statement.js

-  if (this.hasPlugin("asyncGenerators") && this.state.inAsync && this.isContextual("await")) {
-      forAwait = true;
-      this.next();
+  if (this.isContextual("await") && this.state.inAsync) {


Maybe switch the order here like it was before, checking the bool value first is maybe also faster in case it is false?
Or did you have something in mind by changing the order.

danez · 2016-11-11T09:18:33Z

src/parser/statement.js

-      if (this.hasPlugin("asyncGenerators") && this.eat(tt.star)) isGenerator = true;
+      if (this.match(tt.star)) {
+        this.expectPlugin("asyncGenerators");
+        this.next();


We have this pattern quite often if (match) expectPlugin(), next() So maybe we could make this easier in future by doing:

if (this.expectPluginIfMatch("plugin", tt.start)) { is = true }

What do you think?
But should do that anyway later, to not make this PR even more bigger.

danez · 2016-11-11T09:21:18Z

src/parser/util.js

+      if (pluginInfo.babelSyntaxPlugin) {
+        pointers.push(`babel-plugin-${pluginInfo.babelSyntaxPlugin}`);
+      }
+      if (pluginInfo.babelTransformPlugins) {


Should we make babelTransformPlugins mandatory to be an array, so we don't have to check here?
And also as long as we have syntax-plugins for everything, maybe also remove the if for it?

Yeah we do have syntax plugins for every transform (otherwise we would just remove the syntax plugins entirely)

hzoo · 2016-12-15T21:32:15Z

src/plugins/directory.js

+
+export const asyncGenerators = {
+  babelSyntaxPlugin: "syntax-async-generators",
+  babelTransformPlugins: ["transform-regenerator"]


we should recommend the new transform, transform-async-generators

hzoo · 2016-12-15T21:32:51Z

src/plugins/directory.js

+
+export const classConstructorCall = {
+  babelSyntaxPlugin: "syntax-class-constructor-call",
+  babelTransformPlugins: ["transform-class-constructor-call"]


This is deprecated so can remove

hzoo · 2016-12-15T21:33:00Z

src/plugins/directory.js

+  babelTransformPlugins: ["transform-decorators"]
+};
+
+// export const dynamicImport = {


this can be added back

hzoo · 2016-12-15T21:33:12Z

src/plugins/directory.js

+
+export const decorators = {
+  babelSyntaxPlugin: "syntax-decorators",
+  babelTransformPlugins: ["transform-decorators"]


i guess recommend transform-legacy atm?

hzoo · 2016-12-15T21:35:27Z

src/parser/util.js

+    const leadingPointersOr = pointers.length ? ` ${pointers.join(", ")} or ` : " ";
+
+    this.unexpected(null, `This experimental syntax requires the ${name} parser plugin. `
+      + `Check out${ leadingPointersOr }the parserOpts.${name} Babel option.`);


it's an array so the wording is off (maybe we shouldn't recommend that option).

{ parserOpts: { plugins: [] } }

We should change it so that Babel users will use the transform plugin, and babylon users the syntax, and for Babel users not to use both.

motiz88 · 2016-12-16T07:08:04Z

@hzoo so basically - an enhanced error object, whose shape will be part of Babylon's public API, and which we will then detect and translate on the Babel side. Right? My choice for this would either be an ad-hoc extra property e.g. `missingBabylonPlugin` (and duck typing on that in Babel) or just full blown VError with its first-class support for metadata. What do you think? I'll only have my computer back from repairs next week but let's maybe hash out the details here until I can write actual code.

…

On Dec 15, 2016 23:36, "Henry Zhu" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/parser/util.js <#178>: > + const pluginInfo = plugins[name]; + const pointers = []; + if (pluginInfo) { + if (pluginInfo.babelSyntaxPlugin) { + pointers.push(`babel-plugin-${pluginInfo.babelSyntaxPlugin}`); + } + if (pluginInfo.babelTransformPlugins) { + pointers.push(...pluginInfo.babelTransformPlugins.map( + (shortName) => `babel-plugin-${shortName}` + )); + } + } + const leadingPointersOr = pointers.length ? ` ${pointers.join(", ")} or ` : " "; + + this.unexpected(null, `This experimental syntax requires the ${name} parser plugin. ` + + `Check out${ leadingPointersOr }the parserOpts.${name} Babel option.`); We should change it so that Babel users will use the transform plugin, and babylon users the syntax, and for Babel users not to use both. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#178>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACJHpRgemrtssWZrqvq6Q8_RHqzbVjoLks5rIbLWgaJpZM4KXfLp> .

motiz88 · 2017-02-14T22:18:40Z

@hzoo I saw your comment on #300 mentioning this feature. Totally willing to pick this up again. Can you please confirm that the approach I outlined above is how you'd prefer this to be handled? Or do you have something else in mind?

hzoo · 2017-02-14T22:24:26Z

Hmm yeah it would be interesting to just expose metadata that says the babylon plugin that could be applied to not error.

2 users of babylon: either a tool or a Babel user. I would focus on the Babel user since the tool user would be more likely to understand how to enable a plugin anyway?

motiz88 · 2017-02-14T22:29:52Z

Btw just resolved the obvious conflicts for now. Will do a more complete update soon.

hzoo · 2017-02-14T22:33:35Z

src/plugins/directory.js

@@ -0,0 +1,59 @@
+export const classProperties = {


Thinking about this again...

most if not all of the plugins have a corresponding babel plugin that is just the same name.

objectRestSpread -> transform-object-rest-spread
asyncGenerators -> transform-async-generators

the only 2 that don't do that have a corresponding preset as well now

flow -> preset-flow (strip-flow-types)
react -> preset-react (react-jsx)

So we could do this automatically + 2 exceptions?

Most is right. There's also the question of transform vs syntax (even if we ignore the rarer preset and ...-legacy etc) which is still an editorial decision basically

-legacy will be removed (there's a pr to make it the regular one) babel/babel#5283

and ^ I said we always have a transform/syntax version so far at least

Oh yeah, sorry, I was commenting with just a fuzzy recollection of how my own PR worked 🤦‍♂️ . So transform vs syntax is accounted for, by way of both being mentioned in the message.

And now I see your edit above. So yeah, that's a way to encode the same information less explicitly. However I would still kinda prefer to move this out of Babylon if possible, as discussed below.

motiz88 · 2017-02-14T22:36:53Z

@hzoo I read "focus on the Babel user" to mean a relaxing of your suggestion above that

We should change it so that Babel users will use the transform plugin, and babylon users the syntax, and for Babel users not to use both.

So that - showing only the Babel oriented message - would be one simple way to slice it, albeit one that sacrifices some "purity" I guess by coding knowledge about Babel plugins into Babylon (funnily enough, I would be more comfortable with this philosophically if Babylon was in the Babel monorepo... Ooh can we get that in 7.0? :half_joking:)

hzoo · 2017-02-14T22:39:51Z

(funnily enough, I would be more comfortable with this philosophically if Babylon was in the Babel monorepo... Ooh can we get that in 7.0? :half_joking:)

Yeah I've wanted that for a while now (doesn't have to be in 7.0 either but based on time probably so unless someone really wants to do it).

Although I guess it would be a weird error message if the user isn't using Babel but Babylon itself

motiz88 · 2017-02-14T23:09:00Z

it would be a weird error message if the user isn't using Babel but Babylon itself

Yep, that's what I'm least happy about here. I totally get the "focus on Babel users" argument, but Babylon is its own package too and I don't love the idea of making that kind of a second-class story. It's a subtle point though.

To expand on the alternative: It's more work, but if we make the "plugin required" exception positively identifiable to Babel ¹, e.g. via a custom property on the thrown object ², then we can both

put the Babylon option name in the Babylon-thrown message, but then catch it in Babel ³ and rethrow with the appropriate Babel plugin name;
and
remove knowledge from Babylon about which Babel plugins exist/are recommended for each syntax feature. ( & move into babel-core)

1: Or anyone invoking Babylon - i.e this becomes public API.
2: Or subclassing Error but fragile; or bringing in VError but probably overkill; or parsing the actual message prop but eww.
3: In basically just one place, right? babel-core somewhere.

loganfsmyth · 2017-06-07T23:16:53Z

src/parser/util.js

+    this.unexpected(null, `This experimental syntax requires the ${name} parser plugin. `
+      + `Check out${ leadingPointersOr }the parserOpts.${name} Babel option.`);
+  }
+  return true;


Is there a reason to return from this given that the alternative is to throw? Then we could reduce the nesting by just doing if (this.hasPlugin(name)) return; at the top of the function.

loganfsmyth · 2017-06-07T23:19:23Z

src/parser/util.js

+        ));
+      }
+    }
+    const leadingPointersOr = pointers.length ? ` ${pointers.join(", ")} or ` : " ";


How would people feel about skipping this pointers logic and having babylon just throw an error with the plugin name in the message, and a .missingPlugin property?

They we can have the try/catch in babel-core catch the exception and handle mapping the error to a Babel plugin? That way we don't need Babylon to be aware of Babel stuff.

Yeah that was my thoughts dono if it was in the notes. Babylon would just throw and babel-core would pick that up and throw the real error message since you might be using babylon for something else (prettier, babel-eslint) that is unrelated to using a plugin

Point out when a plugin needs to be enabled for encountered syntax (b…

df7868c

…abel#169)

motiz88 added i: enhancement Tag: Polish labels Oct 15, 2016

motiz88 added 2 commits October 15, 2016 17:23

Merge remote-tracking branch 'upstream/master' into expect-plugin

feae64b

Remove let node redefinition in parseExprAtom

0f6bc54

Merge remote-tracking branch 'upstream/master' into expect-plugin

f7559d1

# Conflicts: # test/fixtures/experimental/no-async-generators/error-without-plugin/options.json

danez approved these changes Oct 17, 2016

View reviewed changes

motiz88 added 6 commits October 17, 2016 16:05

Merge remote-tracking branch 'upstream/master' into expect-plugin

d1fd9c2

Make expectPlugin return true on success

fbcdcb8

Convert some async generators paths to use expectPlugin

b73413f

Use expectPlugin() when parsing function.sent metaproperty

041849c

Add more expectPlugin tests

e95ff46

Add test for function.sent error message

891038e

danez reviewed Nov 11, 2016

View reviewed changes

hzoo reviewed Dec 15, 2016

View reviewed changes

src/plugins/directory.js

babelTransformPlugins: ["transform-decorators"]

};

// export const dynamicImport = {

Copy link

Member

hzoo Dec 15, 2016

Choose a reason for hiding this comment

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

this can be added back

hzoo reviewed Dec 15, 2016

View reviewed changes

hzoo mentioned this pull request Jan 16, 2017

Don't parse class properties without initializers when classProperties plugin is disabled, and Flow is enabled #300

Merged

Merge branch 'master' into expect-plugin

880fbdd

hzoo reviewed Feb 14, 2017

View reviewed changes

hzoo added the Priority: High label Mar 30, 2017

nathanhammond mentioned this pull request Jun 7, 2017

Better error message on unexpected token when it's because a plugin isn't turned on #169

Closed

loganfsmyth reviewed Jun 7, 2017

View reviewed changes

hzoo mentioned this pull request Aug 3, 2017

Syntax Error: add message with the plugin that should be enabled #658

Merged

3 tasks

hzoo closed this in #658 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point out when a plugin needs to be enabled for encountered syntax (#169) #178

Point out when a plugin needs to be enabled for encountered syntax (#169) #178

motiz88 commented Oct 14, 2016 •

edited by hzoo

Loading

motiz88 commented Oct 15, 2016 •

edited

Loading

codecov-io commented Oct 15, 2016 •

edited

Loading

motiz88 commented Oct 16, 2016

motiz88 commented Oct 17, 2016 •

edited

Loading

danez commented Oct 17, 2016 •

edited

Loading

motiz88 commented Oct 17, 2016 •

edited

Loading

danez commented Oct 17, 2016

motiz88 commented Oct 17, 2016

danez Nov 11, 2016

danez Nov 11, 2016

danez Nov 11, 2016

danez Nov 11, 2016

danez Nov 11, 2016

hzoo Feb 14, 2017

hzoo Dec 15, 2016

hzoo Dec 15, 2016

hzoo Dec 15, 2016

hzoo Dec 15, 2016

hzoo Dec 15, 2016

hzoo Dec 15, 2016

motiz88 commented Dec 16, 2016 via email

motiz88 commented Feb 14, 2017

hzoo commented Feb 14, 2017 •

edited

Loading

motiz88 commented Feb 14, 2017

hzoo Feb 14, 2017 •

edited

Loading

motiz88 Feb 14, 2017

hzoo Feb 14, 2017 •

edited

Loading

motiz88 Feb 14, 2017 •

edited

Loading

motiz88 commented Feb 14, 2017 •

edited

Loading

hzoo commented Feb 14, 2017

motiz88 commented Feb 14, 2017 •

edited

Loading

loganfsmyth Jun 7, 2017

loganfsmyth Jun 7, 2017

hzoo Jun 9, 2017

Point out when a plugin needs to be enabled for encountered syntax (#169) #178

Point out when a plugin needs to be enabled for encountered syntax (#169) #178

Conversation

motiz88 commented Oct 14, 2016 • edited by hzoo Loading

motiz88 commented Oct 15, 2016 • edited Loading

codecov-io commented Oct 15, 2016 • edited Loading

Codecov Report

motiz88 commented Oct 16, 2016

motiz88 commented Oct 17, 2016 • edited Loading

danez commented Oct 17, 2016 • edited Loading

motiz88 commented Oct 17, 2016 • edited Loading

danez commented Oct 17, 2016

motiz88 commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motiz88 commented Dec 16, 2016 via email

motiz88 commented Feb 14, 2017

hzoo commented Feb 14, 2017 • edited Loading

motiz88 commented Feb 14, 2017

hzoo Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

motiz88 Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

motiz88 commented Feb 14, 2017 • edited Loading

hzoo commented Feb 14, 2017

motiz88 commented Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motiz88 commented Oct 14, 2016 •

edited by hzoo

Loading

motiz88 commented Oct 15, 2016 •

edited

Loading

codecov-io commented Oct 15, 2016 •

edited

Loading

motiz88 commented Oct 17, 2016 •

edited

Loading

danez commented Oct 17, 2016 •

edited

Loading

motiz88 commented Oct 17, 2016 •

edited

Loading

hzoo commented Feb 14, 2017 •

edited

Loading

hzoo Feb 14, 2017 •

edited

Loading

hzoo Feb 14, 2017 •

edited

Loading

motiz88 Feb 14, 2017 •

edited

Loading

motiz88 commented Feb 14, 2017 •

edited

Loading

motiz88 commented Feb 14, 2017 •

edited

Loading