Breaking: Use ESTree export node types in modules (fixes# 263)

[soda0289]

We currently do not rename the import node type in modules. Also we do not rename all the export statement types, only ones that are used to export functions or variables. Named exports or export all statements are not converted. I think it makes more sense to follow the ESTree spec as it allows for eslint rules in the eslint-plugin-import to work correctly.

[JamesHenry]

Seems reasonable! Have you actually tried this with the eslint import plugin out of interest?

[soda0289]

I have tried the plugin and it no longer crashes since we changed TSModuleBlock to use body instead of statements. #218

I just tested the rule import/prefer-default-export and it seems to work as expected for namespaces and normal exports. Even thou typescript complains with:
error TS1319: A default export can only be used in an ECMAScript-style module.

[JamesHenry]

@soda0289 Does any of the AST alignment work affect this change?

[soda0289]

@JamesHenry This should be good. I haven't tested what babylon currently does but it should use the normal export/import nodes

[JamesHenry]

I just tested the rule import/prefer-default-export and it seems to work as expected for namespaces and normal exports. Even thou typescript complains with:
error TS1319: A default export can only be used in an ECMAScript-style module.

Isn't this a big deal? TypeScript won't compile your program if you go one way, and ESLint won't pass if you go the other?

[soda0289]

Yes the default export is bad example since its invalid typescript but the rule still works. Typescript still parses and compiles with warnings. Having an export is valid and right now eslint rules will not know of exports or default exports inside of Modules/Namespaces because we use a custom node type.

[JamesHenry]

So, just to confirm 100%, there will not be a conflict between removing the TypeScript warning and fixing the linting error in that case? i.e. Our users will be able to have clean TypeScript builds and linting tasks using this code?

[soda0289]

Yes to my knowledge there will no conflict between any rules dealing with import/export and the AST we would produce with this change. There will be a warning from typescript if you use default export in a namespace/module but it will still compile to JS. If you had a rule that enforced at least one default export you could export the namespace or module as the default

[JamesHenry]

I have just tested this and unfortunately, you do end up in the position I am asking about where you are stuck between TypeScript being happy and ESLint being happy:

declare module "foo" {
  export type T = true
}

or 

declare module "foo" {
  export const T = 'yo'
}

or

module foo { // note this should be an Identifier, not a string for non-ambient
  export const T = 1
}

or

namespace foo {
  export const T = 1
}

...will all produce false negatives on the prefer-default-export rule.

If we do make this change, we should definitely add a note on the README. I know it comes from a non-core rule, but still I think it is common enough that it could trip people up.

When ESLint 4 lands, people will also finally be able to add eslint-disable comments to these blocks, which is good....

[soda0289]

I see what you mean. Where if you fix the problem, by adding the word default, it produces a non valid typescript file. Although you can fix the issue by adding a second named export or exporting the entire namespace as the defualt export. I was not too sure on the extact intent of the rule, it is working as stated, but it should ignore namespaces and modules to overcome these problems. I do not think every rule needs this fix and this change will allow other rules to work inside namespace and modules.

[soda0289]

Updating the README is a good idea. We can also remove the note on eslint-plugin-react since I do not know of any issues currently in that rule.

[eslintbot]

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

• 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.)