fix: wrap interface in ExportNamedDeclaration if necessary (fixes #269

[despairblue]

No description provided.

[eslintbot]

Thanks for the pull request, @despairblue! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

• Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[eslintbot]

LGTM

[soda0289]

Can you use fixExports() from nodeUtils. It will wrap the node in export and handles default case as well. Take a look at function declaration or class declaration for examples.

To check if an interface is abstract:
nodeUtils.hasModifier(SyntaxKind.AbstractKeyword, node) Should do the trick. I think we can just have a property called abstract.

Thanks

[eslintbot]

LGTM

[despairblue]

@soda0289 done.

The diffs for the other tests are a bit bigger because I used tools/update-typescript-tests.js to generate them. I also fixed tools/update-typescript-tests.js to use the same getRaw implementation the tests use and added a missing shelljs.echo call.

Abstract interfaces are not valid TypeScript btw. But I think they added the test to make sure the parser can handle that, so they can output a useful error message like 'abstract' modifier can only appear on a class, method, or property declaration. instead of Expected ClASS or STRING. Instead found ABSTRACT.

[soda0289]

@despairblue Thanks for making the changes.

If abstract interfaces are not valid typescript I'm not sure we should support them or the very least only set abstract: true when it is present and not set abstract: false. I'll let @JamesHenry review just to make sure this change makes sense.

[despairblue]

I'm happy with whatever choice you make.

That said, my understanding is that this is an AST to AST converter, so it should convert the AST as accurately as possible and should not make assumptions about it's semantics as a program.

But again, it's your choice, we just would need to blacklist some tests from the typescript test suite, no biggie :)

[despairblue]

@soda0289 @JamesHenry is there anything I can do to help this get in? I could remove the abstract if that's the problem.

[soda0289]

I'm fine with using abstract. I think we are also going to change classes to use a property abstract now as well instead of a new node type. I will merge this soon.

[soda0289]

@despairblue There will be quite a few changes coming to the AST as we try to align with the AST from the new typescript babylon plugin. You can check the issues here:
https://github.com/JamesHenry/tsep-babylon-test/issues