Test: missing tests for modules & classes (refs #72)

[xjamundx]

Added tests for the following:

• export default class extends bar {};
• export default class {};
• export default class foo extends bar {};
• export default class foo {};
• export class foo extends bar {};
• export class foo {};

Fixed the following:

• Provided a fix for export default class foo extends bar {}; such that it would not throw

Need to check the following:

• AST for export default class {}; in class-default-anonymous.result.js
• AST for export default class foo extends bar {}; in class-default-anonymous-extends.result.js

The ASTs are tricky and need to be verified as accurate, because they were generated by espree.

[xjamundx]

This will clash with 731d54f a bit

[caridy]

yes, that has to be fixed in espree and esprima. For anonymous classes and functions for export default we are using expressions, but really they are not expressions, they are nameless declarations. We got to consensus on that few days ago to avoid introducing HoistableDeclaration. This all means we need to fix part of the pieces I put together in the previous PR.

[xjamundx]

Thanks so much for your work on this @caridy Really amazing work! Do you want me to look into this last class issue?

[nzakas]

Can you both yeah take a look at #73 for my changes? I'd like to use that as a bad before making further changes.

[xjamundx]

Okay will look at fixing that last test tomorrow!

[xjamundx]

Looking quickly at export default class extends bar {}; to see if there's an easy fix.

[xjamundx]

Initially appears to be related to parseClassDeclaration() vs parseClassExpression(). Only in the case of modules we do this with parseSourceElement:

case "class":
    if (allowClasses) {
        return parseClassDeclaration();
    }
    break;

Inside of parseClassDeclaration we assume a name with this bit of code (which i think chokes up on finding extends instead):

expectKeyword("class");
id = parseVariableIdentifier();
if (matchKeyword("extends")) {
   lex();
   superClass = parseLeftHandSideExpressionAllowCall();
}

[nzakas]

What's your definition of "not working"? I'm not clear on the bug you're trying to fix.

[caridy]

Here is a related issue:

https://github.com/eslint/espree/blob/master/tests/fixtures/ecma-features/modules/export-default-function.result.js#L7

when doing export default function () {} that's not an expression as we have it today (my bad), it should be a function declaration with no name. Probably we should fix the function declaration parser first, to support that case (which is spec'd as HoistDeclaration), and then we can do the same for classes, where we can have a class that has no name but it is still a class declaration exported as default.

[caridy]

/cc @jeffmo

[xjamundx]

@nzakas the issue is that export default class extends foo {} would throw (and I originally just included a test that validated the error)
@caridy yeah you know way more about the internal of how classes should work. the latest commit includes a workaround by parsing it as an expression. we could also make it a ClassDeclaration with a null name. I have a hunch it's not the only class AST that isn't 100% perfect :)

Let me know what to change!

[xjamundx]

Updated the PR description with clearer info and asks.

[nzakas]

Ah, I see, so this is the unnamed function declaration discussion. Got it. (Ref: estree/estree#38)

So the change here is to allow a null ID for both function and class declarations.

[caridy]

@nzakas correct.

[xjamundx]

I feel like this is missing a declaration :|

[caridy]

ha, that's a bug. related to what I mentioned before.

[xjamundx]

awesome i'll try to take a stab at this tonight :)

[xjamundx]

Okay took care of the ClassDeclaration stuff. I'm less familiar with the FunctionDeclaration stuff! I think the ASTs make sense now :)

[xjamundx]

Ahh the name/id stuff....fixing

[xjamundx]

Ok, should be good to go. Let me know if you need anything else on this (like FunctionDecl stuff)...

[nzakas]

Cool, I'm hanging out in #eslint on Freenode as I try to get all of this merged and into ESLint

[nzakas]

This isn't quite right because parseClassDeclaration() normally must have an identifier. So you really need to pass in a flag specifying whether the ID is optional or required.

[xjamundx]

Let me know if you have a specific class declaration that breaks, at the moment I think this code-path may only be called in export statements, so maybe it works by accident? maybe.

[xjamundx]

I'm going to be out for most of the day taking kids to the zoo, but may be able to help later!

[nzakas]

That's okay, I'll just merge and fix it. Thanks!

[nzakas]

Just FYI, the problem case is:

class {}

This should throw an error because class declarations require names, but this change parsed it okay.