-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good 👍
ImportDeclaration(node) { | ||
if ( | ||
node.source.value === 'react' && | ||
node.specifiers[0].type === 'ImportDefaultSpecifier' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably need to guard against no specifiers
Is just forbidding |
I might prefer the rule over Also, would this be under the typescript namespace as well? |
I think this can be applied in JS or TS, it works the same either way. Disallowing |
@@ -0,0 +1,36 @@ | |||
# Prefer React import be name-spaced. (prefer-name-spaced-react) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to fill this in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, sorry, I forgot i was just looking for general approach approval at first. Will clean this all up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we actually want the opposite behaviour here as per https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-7.html:
We highly recommend applying it (
esModuleInterop
) both to new and existing projects. For existing projects, namespace imports (import * as express from "express"; express();) will need to be converted to default imports (import express from "express"; express();).
|
||
context.report({ | ||
node, | ||
message: `Prefer name-spaced React import. (import * as React from 'react';).`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we actually want the opposite here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious whether TS will complain about the namespace import for React with that flag applied. My gut is that a namespace import is still more correct for React. In express, it made a lot less sense, because the namespace import got you a function, where it would always give you an object in “true” modules. React, in contrast, uses module.exports as an object that just exposes named properties, so a namespace import is more appropriate.
superseded by #262 |
closes Shopify/web-configs#115
If I can get approval on the general approach and if we want this rule, I'll fill in the docs, configs, changelog, etc...