Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TypeScript] Support default exports in TSExportAssignment. #1528

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

joaovieira
Copy link
Contributor

@joaovieira joaovieira commented Oct 30, 2019

Allows TypeScript export assignments of default identifiers and/or mixed merged declarations. E.g.:

declare const foobar: number;
export = foobar;
declare function foobar(): void;
declare namespace foobar {
  type MyType = string
}
export = foobar;
import { foobar } from './module';
export = foobar;
// No longer reports missing default export
import foobar, { MyType } from './foobar';

Fixes: #1527

moduleDecl.body.body.forEach((moduleBlockNode) => {
if (exportedDecls.length === 0) {
// Export is not referencing any local declaration, must be re-exporting
m.namespace.set('default', captureDoc(source, docStyleParsers, n))
Copy link
Contributor Author

@joaovieira joaovieira Oct 30, 2019

Choose a reason for hiding this comment

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

I'm not sure about n here. Would it be worth looking through the m.imports to see if any matches the exported name? We don't do that on regular exports. E.g.:

import { test } from './test';
export { foo }; // all good...

@coveralls
Copy link

coveralls commented Oct 30, 2019

Coverage Status

Coverage increased (+2.5%) to 97.815% when pulling 381267a on joaovieira:extend-ts-export-assignment into 9516152 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 96.151% when pulling 86d2eff on joaovieira:extend-ts-export-assignment into 112a0bf on benmosher:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 96.151% when pulling 86d2eff on joaovieira:extend-ts-export-assignment into 112a0bf on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 96.151% when pulling 86d2eff on joaovieira:extend-ts-export-assignment into 112a0bf on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 96.151% when pulling 86d2eff on joaovieira:extend-ts-export-assignment into 112a0bf on benmosher:master.

@JounQin
Copy link
Collaborator

JounQin commented Nov 11, 2019

It should read about esModuleInterop option or add an equal option for this plugin instead of hard coding the logic IMO.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2019

@JounQin can you elaborate?

TS's module system is broken without synthetic imports and esModuleInterop enabled; I'm completely comfortable telling people they need to enable those if they want stuff to work properly.

@Maxim-Mazurok
Copy link
Contributor

Maxim-Mazurok commented Mar 19, 2020

What a great PR! I was searching for a solution all day, finally gave up and started to dig into the sources. Strangely enough, this PR didn't come up while I was searching for similar issues.
So, I started coding basically the same thing: #1689

@JounQin I agree with you, in my PR I'm reading from tsconfig.json. But, there might be multiple tsconfigs and they might include/exclude specific files. So, the best approach would be to first, figure out which tsconfig is covering file where the import is made and then check if that config has esModuleInterop enabled.
Here's an example of code from eslint-import-resolver-typescript that does kind of similar thing:

mappers = configPaths!
  // turn glob patterns into paths
  .reduce<string[]>(
    (paths, path) => paths.concat(isGlob(path) ? globSync(path) : path),
    [],
  )
  .map(loadConfig)
  .filter(isConfigLoaderSuccessResult)
  .map(configLoaderResult => {
    const matchPath = createMatchPath(
      configLoaderResult.absoluteBaseUrl,
      configLoaderResult.paths,
    )

    return (source: string, file: string) => {
      // exclude files that are not part of the config base url
      if (!file.includes(configLoaderResult.absoluteBaseUrl)) {
        return undefined
      }

      // look for files based on setup tsconfig "paths"
      return matchPath(source, undefined, undefined, extensions)
    }
  })

That being said, we should either do the right thing and figure out how to handle complex cases. Or, we can do the simple thing and only cover simple case (only root tsconfig) or introduce new leg in parserOptions that will also act globally but will be more explicit than assuming that everybody has only one tsconfig.

Also, I can't disagree with @ljharb about assuming that everyone uses esModuleInterop nowadays. It seems to be a pretty popular flag to use. And in this case, I would be very happy to see this PR merged, thanks!

]
const exportedDecls = ast.body.filter(({ type, id, declarations }) =>
declTypes.includes(type) &&
(id && id.name === exportedName || declarations.find(d => d.id.name === exportedName))
Copy link
Contributor

@julien1619 julien1619 May 13, 2020

Choose a reason for hiding this comment

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

Hey! Here declarations is undefined unless type === 'VariableDeclaration' so it crashes the linter in some cases. I'll create another PR to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

ljharb pushed a commit to julien1619/eslint-plugin-import that referenced this pull request May 31, 2020
@richard-lopes-ifood
Copy link

Nice fixing 🚀! When is it going to be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Handle TSExportAssignment exports other than namespaces
7 participants