-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Increase the severity of jsdoc/no-undefined-types
.
#21942
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
Increase the severity of jsdoc/no-undefined-types
.
#21942
Conversation
@@ -93,6 +93,7 @@ module.exports = { | |||
...temporaryWordPressInternalTypes, | |||
...temporaryExternalTypes, | |||
'void', | |||
'JSX', |
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.
JSX is in global namespace. That's why it is added 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.
Do you know at what point this would come "in scope" ? I guess if it were an issue, the TypeScript build would be complaining about it?
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.
The members of JSX namespace are all types or interfaces. They're imported when we import react
. So, TypeScript will only complain we use JSX elements before import something from 'react'
like:
/**
* @return {JSX.Element} a component
*/
function f() {}
const React = require('react');
I'm not sure it would be even an error if hoisting happens. It's also against our coding convention.
In conclusion, it's almost impossible to happen.
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.
One of the worries I might have is that for createElement
in particular, we automate this via a Babel transform, so that the source doesn't actually include an import
statement.
See: https://github.com/WordPress/gutenberg/tree/master/packages/babel-plugin-import-jsx-pragma
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've tested by adding a new file without importing React
:
// a11y/src/index.js
export { X } from './x';
// a11y/src/x.js
/**
* Test if it works without TypeScript complaining.
*
* @return {JSX.Element} React component
*/
export const X = () => <div />;
It works without any complaints. We're good to go.
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.
The changes look good. I think we'll need to mark this as a breaking change, however, since this is part of the default configuration and there's no guarantee that upgrading won't require a consuming project to make changes in their code.
Could you add a note to the package's CHANGELOG.md, under a new ### Breaking Changes
heading?
bd98c8d
to
983b993
Compare
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.
Might want to double-check https://github.com/WordPress/gutenberg/pull/21942/files#r417366187 . Also, there's a merge conflict. Otherwise, it looks good 👍
983b993
to
ff093e1
Compare
Description
Increase the severity of
jsdoc/no-undefined-types
.How has this been tested?
No warnings at lint.
Screenshots
N/A
Types of changes
Bug fix
Checklist: