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

[no-namespace] Allow module when no-namespace is turned on #423

Closed
JaceHensley opened this issue Apr 9, 2019 · 4 comments
Closed

[no-namespace] Allow module when no-namespace is turned on #423

JaceHensley opened this issue Apr 9, 2019 · 4 comments
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended

Comments

@JaceHensley
Copy link

Repro

parser: '@typescript-eslint/parser',
plugins: ['@typescript-eslint'],
extends: [
  'plugin:@typescript-eslint/recommended'
],
// Error, I want this to happen
namespace testingNamespace {
  ...
}

// Error, this should be allowed
module testingModule {
  ...
}

Additional Info

Setting up TS with babel-loader, babel doesn't support the namespace name {} syntax and suggests using the module name {} syntax. But I can't seem to be able to enforce that with the no-namespace lint rule. I would expect to be able to disallow namespace but still allow module (without the declare keyword before it too)

Versions

package version
@typescript-eslint/eslint-plugin 1.6.0
@typescript-eslint/parser 1.6.0
TypeScript 3.4.1
ESLint 5.16.0
node 8.9.4
npm 6.6.0
@JaceHensley JaceHensley added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Apr 9, 2019
@bradzacher
Copy link
Member

AFAIK without declare, both module and namespace work the same way, and can be used interchangeably.

module Foo {
    export const x = '';
}
namespace Foo {
    export const y = '';
}

namespace Bar {
    export const x = '';
}

Foo.x;
Foo.y;

repl

It's also worth noting that the parser makes no differentiation between the two.


I have two questions for you

  • why are you looking to use namespace/module?
    • You can do a better job of namespacing by moving the namespaced types into their own file, and importing the file.
  • why are you looking to use babel-loader?
    • You'll do a lot better with ts-loader because babel-loader doesn't do any type checking afaik.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Apr 9, 2019
@JaceHensley
Copy link
Author

Our current usage of namespace is like this:

namespace name {
  export type MyFirstType = {...}

  export type MySecondType = {...}
}

We also have a few those in a single file cause that's just how we are organizing it. So we could change that to not use namespace or module but module is supported by babel/ES.

We use babel-loader + fork-ts-checker-webpack-plugin. There's a small perf boost with babel-loader + fork-ts-checker-webpack-plugin over ts-loader (compileOnly: true) + fork-ts-checker-webpack-plugin. And we also get to hook into the large ecosystem of babel plugins, presets, tutorials.

That is interesting that both show up the same in the AST

@JaceHensley
Copy link
Author

Hmm so checking out the babel playground it seems like module name { and namespace name { are treated the same, that's very odd that they suggest moving to module syntax. Looks like we'll need to migrate away from that pattern. Thanks for helping we walk through that

@bradzacher
Copy link
Member

You can use ASTExplorer to switch to the actual typescript parser, and see that even at the typescript AST level, there is no differentiation between the two.

Using ts-loader doesn't preclude you from also using babel-loader! There's a perf hit ofc, though I would personally use ts-loader as it's guaranteed to work with TS syntax because it uses typescript itself to transpile (unlike babel, which is a custom transpilation implementation).
Unless you're getting a large perf improvement, I wouldn't have thought that the tradeoffs and potential bugs are worth it.

Note: this is the reason that it's not currently supported in babel: babel/babel#8244

Namespaces imply one of two things:

  • Global code structuring (as they'll merge across files)
  • Extra layers of nesting within a module

Neither case seems to be ideal, nor does it feel like the direction the JavaScript community is taking. But more than that, there's a technical constraint which is that Babel's single-file-at-a-time model doesn't really support cross-file resolution.

@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended and removed awaiting response Issues waiting for a reply from the OP or another party labels Apr 9, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended
Projects
None yet
Development

No branches or pull requests

2 participants