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

create-react-app enforces @typescript-eslint/no-namespace, a style rule #7651

Open
szhu opened this issue Sep 6, 2019 · 12 comments

Comments

@szhu
Copy link

commented Sep 6, 2019

Describe the bug

  1. create-react-app --typescript
  2. Add a namespace in a file
  3. I get the error: ES2015 module syntax is preferred over custom TypeScript modules and namespaces.eslint(@typescript-eslint/no-namespace). There seems to be no way to turn off this rule, according to #3886.

Why this is a bug:

From #2157 (comment):

Our rules are specifically picked to not enforce style, and to only find logical mistakes.

Whether or not a namespace is used is most definitely not a choice of style and not a logical mistake. Please remove this rule or give me an option to remove this rule.

@heyimalex

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

Could you comment out that line directly in node_modules and see if compilation works as you'd expect? This landed in #6513 right when we added native ts support, so I'm thinking maybe there's an actual reason it's there. But maybe not.

@szhu

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

Ah:

SyntaxError: .../xxx.tsx: Namespaces are not supported.

Which part of the build process are namespaces not supported by?

@heyimalex

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

We use babel to compile typescript (vs directly with tsc), so probably there. I just checked here and at a glance it looks like support is being added, at least for type-only namespaces (we're usually pinned to a slightly out of date babel, so maybe why you don't see the same error message).

If you want to do the legwork to land the requisite changes I'd be happy to mentor/review! I can write out some steps, just let me know if you're interested.

@szhu

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

Ah, it's Babel.

Also, I tried this with the newest version of CRA. I get the same lint error, but after disabling it according to the above instructions, I now get:

SyntaxError: /Users/Sean/test-app/src/App.tsx: Namespace not marked type-only declare. Non-declarative namespaces are only supported experimentally in Babel. To enable and review caveats see: https://babeljs.io/docs/en/babel-plugin-transform-typescript

@heyimalex

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

Oh lol, then no changes needed? Does a type-only namespace work for your usecase or nah? If not then there's not much we can do until this lands (as non-experimental) in babel. If it is but the lint rule is still firing then we can fix that.

@szhu

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

So, it looks like disabling the lint error can at least get declare namespace to work. I'd be happy to make that change.

Please let me know what I can do! Here are the changes that I think I need to make:

  • In eslint-config-react-app, remove the @typescript-eslint/no-namespace rule.
  • Update react-scripts to use that updated version of eslint-config-react-app
  • (Possibly?) Update in react-scripts, run babel with the allowNamespaces option. (This enables partial support for non-declare-only namespaces, see https://babeljs.io/docs/en/babel-plugin-transform-typescript) Let me know if this is a change we can/should make.
  • Update create-react-app to use that updated version of react-scripts.

Let me know if this seems correct and if you have any pointers!

@szhu

This comment has been minimized.

Copy link
Author

commented Sep 7, 2019

Btw, there's one warning I have left. At this point I should probably explain my use case:

namespace Counter {
  interface Props {
    value: number
    setValue: (value: number) => void
  }
}

const Counter: React.FC<Counter.Props> = (props) => {
  return <div>{value}</div>
}

export default Counter

Basically, I don't want to export/use props separately from the component.


For this example, the proposed fixes will make it work! But I'm still left with this warning:

Compiled with warnings.
  'Counter' is already defined  no-redeclare

Let me know what you think. Should no-redeclare also be removed? Is the props organization convention a bad one? Is there a way to achieve the same effect without namespaces? Also let me know if you think this is worthy of a separate issue.

@heyimalex

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

Hm, it's friday night for me so I'll have to take a look this weekend. I'm pretty sure we don't want to get rid of no-redeclare, and I'm actually kinda surprised that it's even firing for what is essentially a type definition since I'm pretty sure I've done similar things in the past. Maybe it just sees namespaces differently.

RE: Your use case: That's a pretty neat pattern! I'm not an expert on the best practices, but I've read a lot of type definitions on DefinitelyTyped and generally I've seen a separate CounterProps export. You can also do React.ComponentProps<typeof Counter> I think. But I understand the ergonomic appeal of having type/constructor-associated access to props, and I'd like to get this sorted either way.

@heyimalex

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

Oh have you tried adding declare in front of your namespace definition? I can actually see why it would complain about re-declaring since it doesn't know that it's types-only, so it has to be an actual callable thing.

@szhu

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

Hey no worries, take your time! Thanks for your input on this.

CounterProps

Not a bad idea. I feel like that's the de-facto standard, and my company uses this in our codebase so I have a lot of experience with this and know it scales fairly well, but it just seems like just a little extra work to maintain (i.e., to rename a component, you have to rename-refactor two things instead of one).

React.ComponentProps<typeof Counter>

Ah yes, I keep forgetting and then remembering this one, since it's longer to type out. I might actually just go with this one in my project, because I don't actually need to access the type for component props very often.


Okay so back to solving this lint issue. To answer your questions above, I checked and:

  • no-redeclare gets triggered even for declare namespace.
  • no-namespace gets triggered even for declare namespace.
  • No Babel flag changes are needed for declare namespace to compile.
@cordasfilip

This comment has been minimized.

Copy link

commented Sep 11, 2019

I think this is probably related but now when I do create react-app my-app --typescript and add a console.log(location.href) I get a "Unexpected use of 'location' no-restricted-globals" is there I way to turn tis off because I need to update an project from a few months ago and none of this was an issue in Jun? Also I don't get any issues in VS code because the this lint rule is not picked up at all. If I was able at least to see how many issues I have I might be able to fix them.

@szhu

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

Hey @cordasfilip! I think that this could be an unrelated issue, and I'd recommend creating a new GitHub issue to discuss that. If you think that people reading that issue should read this issue too, you can link to this issue there.

Since I'm already here, here are some thoughts that come to mind:

  • I think VSCode doesn't run eslint on ts files by default, so that's why VSCode isn't showing the errors. Here are some keywords that can help you fix that.

  • If you replace location with window.location, does it work?

  • If debugging is required, it might be helpful if you could list of specific reproduction steps. When you make a new Github issue, the template will prompt you to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.