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

Disable for...of by default, rewrite cases where it matters #12198

Merged
merged 3 commits into from
Feb 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {

plugins: [
'jest',
'no-for-of-loops',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need a whole plugin for this? You can use no-restricted-syntax to block for..of loops with a custom message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I didn’t know that. Happy to take a PR.

'react',
'react-internal',
],
Expand Down Expand Up @@ -56,6 +57,10 @@ module.exports = {
// We don't care to do this
'react/jsx-wrap-multilines': [ERROR, {declaration: false, assignment: false}],

// Prevent for...of loops because they require a Symbol polyfill.
// You can disable this rule for code that isn't shipped (e.g. build scripts and tests).
'no-for-of-loops/no-for-of-loops': ERROR,

// CUSTOM RULES
// the second argument of warning/invariant should be a literal string
'react-internal/no-primitive-constructors': ERROR,
Expand Down
2 changes: 1 addition & 1 deletion dangerfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ fetch(commitURL(parentOfOldestCommit)).then(async response => {

// Show a hidden summary table for all diffs

// eslint-disable-next-line no-var
// eslint-disable-next-line no-var,no-for-of-loops/no-for-of-loops
for (var name of new Set(packagesToShow)) {
const thisBundleResults = results.filter(r => r.packageName === name);
const changedFiles = thisBundleResults.filter(
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"eslint-plugin-babel": "^3.3.0",
"eslint-plugin-flowtype": "^2.25.0",
"eslint-plugin-jest": "^21.6.1",
"eslint-plugin-no-for-of-loops": "^1.0.0",
"eslint-plugin-react": "^6.7.1",
"eslint-plugin-react-internal": "link:./scripts/eslint-rules/",
"fbjs": "^0.8.16",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ function prepareChildrenArray(childrenArray) {
function prepareChildrenIterable(childrenArray) {
return {
'@@iterator': function*() {
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const child of childrenArray) {
yield child;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@ const RCTFabricUIManager = {
let out = '';
out +=
' '.repeat(indent) + info.viewName + ' ' + JSON.stringify(info.props);
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const child of info.children) {
out += '\n' + dumpSubtree(child, indent + 2);
}
return out;
}
let result = [];
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const [rootTag, childSet] of roots) {
result.push(rootTag);
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const child of childSet) {
result.push(dumpSubtree(child, 1));
}
Expand Down
2 changes: 2 additions & 0 deletions packages/react-native-renderer/src/__mocks__/UIManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ const RCTUIManager = {
let out = '';
out +=
' '.repeat(indent) + info.viewName + ' ' + JSON.stringify(info.props);
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const child of info.children) {
out += '\n' + dumpSubtree(child, indent + 2);
}
Expand Down Expand Up @@ -143,6 +144,7 @@ const RCTUIManager = {
indicesToInsert.push([addAtIndices[i], addChildReactTags[i]]);
});
indicesToInsert.sort((a, b) => a[0] - b[0]);
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const [i, tag] of indicesToInsert) {
insertSubviewAtIndex(parentTag, tag, i);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ const ReactNoop = {
const n = timeout / 5 - 1;

let values = [];
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const value of flushUnitsOfWork(n)) {
values.push(...value);
}
Expand All @@ -418,6 +419,7 @@ const ReactNoop = {

flushUnitsOfWork(n: number): Array<mixed> {
let values = [];
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const value of flushUnitsOfWork(n)) {
values.push(...value);
}
Expand All @@ -427,6 +429,7 @@ const ReactNoop = {
flushThrough(expected: Array<mixed>): void {
let actual = [];
if (expected.length !== 0) {
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const value of flushUnitsOfWork(Infinity)) {
actual.push(...value);
if (actual.length >= expected.length) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ describe('ReactIncrementalTriangle', () => {

function simulate(...actions) {
const gen = simulateAndYield();
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (let action of actions) {
gen.next(action);
}
Expand Down Expand Up @@ -407,6 +408,7 @@ ${formatActions(actions)}

function simulateMultipleRoots(...actions) {
const roots = new Map();
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (let rootID of rootIDs) {
const simulator = TriangleSimulator(rootID);
const generator = simulator.simulateAndYield();
Expand Down
6 changes: 3 additions & 3 deletions packages/react-test-renderer/src/ReactTestRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -553,12 +553,12 @@ function findAll(
}
}

for (const child of root.children) {
root.children.forEach(child => {
if (typeof child === 'string') {
continue;
return;
}
results.push(...findAll(child, predicate, options));
}
});

return results;
}
Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,9 @@ function validatePropTypes(element) {
function validateFragmentProps(fragment) {
currentlyValidatingElement = fragment;

for (const key of Object.keys(fragment.props)) {
const keys = Object.keys(fragment.props);
for (let i = 0; i < keys.length; i++) {
const key = keys[i];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strictly saying this is the only case where it matters. But I figured changing TestRenderer code doesn't hurt too.

if (!VALID_FRAGMENT_PROPS.has(key)) {
warning(
false,
Expand Down
1 change: 1 addition & 0 deletions scripts/error-codes/invertObject.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function invertObject(targetObj /* : ErrorMap */) /* : ErrorMap */ {
const result = {};
const mapKeys = Object.keys(targetObj);

// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const originalKey of mapKeys) {
const originalVal = targetObj[originalKey];

Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ async function buildEverything() {

// Run them serially for better console output
// and to avoid any potential race conditions.
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const bundle of Bundles.bundles) {
await createBundle(bundle, UMD_DEV);
await createBundle(bundle, UMD_PROD);
Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1883,6 +1883,10 @@ eslint-plugin-jest@^21.6.1:
version "21.6.1"
resolved "https://registry.yarnpkg.com/eslint-plugin-jest/-/eslint-plugin-jest-21.6.1.tgz#adca015bbdb8d23b210438ff9e1cee1dd9ec35df"

eslint-plugin-no-for-of-loops@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/eslint-plugin-no-for-of-loops/-/eslint-plugin-no-for-of-loops-1.0.0.tgz#a13d91a8f1922f7fefedeab351dc0055994601f6"

"eslint-plugin-react-internal@link:./scripts/eslint-rules":
version "0.0.0"
uid ""
Expand Down