Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

feat: add liferay-portal/no-react-dom-render rule #71

Merged
merged 3 commits into from
Aug 21, 2019

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Aug 20, 2019

Just for fun, I made this one pretty comprehensive (but not exhaustive) in terms of finding bindings to the "react-dom" render() function. I took the simple approach of remembering imports as I see them, and checking CallExpression nodes along the way to see if they correspond to an import. This is in contrast to the alternative approach which would be to scan the entire input accumulating imports and then checking CallExpressions at the very end.

Three reasons for this:

  • It's simpler.
  • import statements usually go at the top of the file, so we will probably always see them first.
  • I suspect, but am not sure, that import bindings have const semantics, and so are affected by the temporal dead zone, which makes it even less likely that they'd be used before we've seen them (you could still use them inside a lazily-executed function, but I think the odds of that are vanishingly small).

Possible add-on to consider:

  • Do want to allow ReactDOM.render inside test/**/*.js directories?

@wincent wincent force-pushed the wincent/no-react-dom-render branch from cb05189 to eadf9b8 Compare August 20, 2019 17:00
* SPDX-License-Identifier: MIT
*/

/* eslint-disable no-for-of-loops/no-for-of-loops */
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 irony.

@wincent wincent requested a review from jbalsas August 20, 2019 17:01
@wincent wincent force-pushed the wincent/no-react-dom-render branch from eadf9b8 to 5876d0b Compare August 20, 2019 19:51
@wincent wincent force-pushed the wincent/no-react-dom-render branch from 5876d0b to 4c55fb5 Compare August 20, 2019 19:52
@wincent wincent marked this pull request as ready for review August 20, 2019 20:14
Discovered while testing in liferay-portal.
@wincent
Copy link
Contributor Author

wincent commented Aug 21, 2019

Going to merge this — tested it in liferay-portal and all is good — and if we decide we want to loop back and add an exception that allows the use of ReactDOM.render in tests, that will be easy enough:

diff --git a/portal.js b/portal.js
index b18ebf5..b097269 100644
--- a/portal.js
+++ b/portal.js
@@ -17,6 +17,14 @@ const config = {
 			jsx: true,
 		},
 	},
+	overrides: [
+		{
+			files: ['**/test/**/*.js'],
+			rules: {
+				'liferay-portal/no-react-dom-render': 'off',
+			},
+		},
+	],
 	plugins: [local('liferay-portal')],
 	rules: {
 		'liferay-portal/no-explicit-extend': 'error',

Although in tests, we might actually want to recommend the use of the render function from @testing-library/react instead.

@wincent wincent merged commit 6f59a06 into master Aug 21, 2019
@wincent wincent deleted the wincent/no-react-dom-render branch August 21, 2019 14:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant