Skip to content

Conversation

sainthkh
Copy link
Contributor

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:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • [N/A] My code has proper inline documentation.
  • [N/A] I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@@ -93,6 +93,7 @@ module.exports = {
...temporaryWordPressInternalTypes,
...temporaryExternalTypes,
'void',
'JSX',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

@sainthkh sainthkh mentioned this pull request Apr 28, 2020
6 tasks
@sainthkh sainthkh added the [Tool] ESLint plugin /packages/eslint-plugin label Apr 28, 2020
@sainthkh sainthkh marked this pull request as draft April 28, 2020 10:43
@sainthkh sainthkh marked this pull request as ready for review April 28, 2020 11:53
Copy link
Member

@aduth aduth left a 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?

@sainthkh sainthkh force-pushed the fix/no-undefined-types-severity branch from bd98c8d to 983b993 Compare April 29, 2020 02:09
Copy link
Member

@aduth aduth left a 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 👍

@sainthkh sainthkh force-pushed the fix/no-undefined-types-severity branch from 983b993 to ff093e1 Compare April 29, 2020 23:57
@aduth aduth merged commit 90d3e6c into WordPress:master Apr 30, 2020
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] ESLint plugin /packages/eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants