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

ESLint: error on missing lint ignore comments #7829

Merged
merged 9 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
7 changes: 7 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ module.exports = {
"@shopify/react-hooks-strict-return": "error",
"@shopify/prefer-module-scope-constants": "error",
"@shopify/jest/no-snapshots": "warn",
"new-cap": [
"error",
{
capIsNewExceptionPattern: "(TEST_|INTERNAL_|HACK_)",
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

},
],
fregante marked this conversation as resolved.
Show resolved Hide resolved
"eslint-comments/require-description": "warn",
"react/no-array-index-key": "error",
"react/no-unstable-nested-components": ["error", { allowAsProps: true }],
"react/forbid-elements": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const VARIABLE_START_INDEX = Symbol("#Variable_start_index");

export interface VariableOptions {
type?: string;
// eslint-disable-next-line @typescript-eslint/no-use-before-define
// eslint-disable-next-line @typescript-eslint/no-use-before-define -- define type interface first
fregante marked this conversation as resolved.
Show resolved Hide resolved
parent?: Variable | undefined;
startIndex: number;
}
Expand Down
23 changes: 13 additions & 10 deletions src/analysis/analysisVisitors/varAnalysis/varAnalysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,19 @@ async function setIntegrationDependencyVars(
contextVars: VarMap,
): Promise<void> {
// Loop through all the integrations, so we can set the source for each dependency variable properly
for (const integrationDependency of extension.integrationDependencies ?? []) {
// eslint-disable-next-line no-await-in-loop
const serviceContext = await makeServiceContextFromDependencies([
integrationDependency,
]);
contextVars.setExistenceFromValues({
source: `${KnownSources.SERVICE}:${integrationDependency.integrationId}`,
values: serviceContext,
});
}
await Promise.all(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only functional change in the PR. This should be safe (and more efficient)

Copy link
Contributor

Choose a reason for hiding this comment

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

We also have an asyncForEach helper so you don't have to use a map that doesn't return anything

Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember that helper, but it looks like it no longer exists?

(extension.integrationDependencies ?? []).map(
async (integrationDependency) => {
const serviceContext = await makeServiceContextFromDependencies([
integrationDependency,
]);
contextVars.setExistenceFromValues({
source: `${KnownSources.SERVICE}:${integrationDependency.integrationId}`,
values: serviceContext,
});
},
),
);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/auth/authUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ export function selectUserDataUpdate({

return {
// TODO: Review Required/Partial in Me type
// https://github.com/pixiebrix/pixiebrix-extension/issues/7725
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion
// https://github.com/pixiebrix/pixiebrix-extension/issues/7725
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion -- email is required
Copy link
Contributor

Choose a reason for hiding this comment

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

For ! comments, I'd expect to see an explanation of how we know it's non null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this one in particular will be fixed soon by @BLoe 's work.

email: email!,
organizationId: organization?.id ?? null,
telemetryOrganizationId: telemetryOrganization?.id ?? null,
Expand Down
5 changes: 0 additions & 5 deletions src/auth/featureFlagStorage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ describe("featureFlags", () => {
});

it("returns true if flag is present", async () => {
// eslint-disable-next-line new-cap
await TEST_overrideFeatureFlags([
"test-flag",
"test-other-flag",
Expand All @@ -53,7 +52,6 @@ describe("featureFlags", () => {
});

it("returns false if flag is not present", async () => {
// eslint-disable-next-line new-cap
await TEST_overrideFeatureFlags(["test-other-flag", "test-other-flag-2"]);
await expect(flagOn("test-flag")).resolves.toBe(false);
});
Expand All @@ -68,7 +66,6 @@ describe("featureFlags", () => {
});

it("does not fetch if flags have been updated recently", async () => {
// eslint-disable-next-line new-cap
await TEST_overrideFeatureFlags(["test-flag"]);
await expect(flagOn("test-flag")).resolves.toBe(true);
expect(appApiMock.history.get).toHaveLength(0);
Expand All @@ -95,9 +92,7 @@ describe("featureFlags", () => {
expect(appApiMock.history.get).toHaveLength(1);

const authData = tokenAuthDataFactory();
// eslint-disable-next-line new-cap
await TEST_setAuthData(authData);
// eslint-disable-next-line new-cap
TEST_triggerListeners(authData);

// New user doesn't have secret flag
Expand Down
3 changes: 1 addition & 2 deletions src/background/auth/authStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ export async function deleteCachedAuthData(serviceAuthId: UUID): Promise<void> {
console.debug(
`deleteCachedAuthData: removed data for auth ${serviceAuthId}`,
);
// OK because we're guarding with hasOwn
// eslint-disable-next-line security/detect-object-injection
// eslint-disable-next-line security/detect-object-injection -- OK because we're guarding with hasOwn
delete current[serviceAuthId];

await oauth2Storage.set(current);
Expand Down
6 changes: 2 additions & 4 deletions src/background/auth/codeGrantFlow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,15 +164,13 @@ async function codeGrantFlow(
}

const json = Object.fromEntries(parsed.entries());
// TODO: Fix IntegrationConfig types
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion -- TODO: Fix IntegrationConfig types
await setCachedAuthData(auth.id!, json);
return json as AuthData;
}

if (typeof data === "object") {
// TODO: Fix IntegrationConfig types
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion -- TODO: Fix IntegrationConfig types
await setCachedAuthData(auth.id!, data);
return data as AuthData;
}
Expand Down
3 changes: 1 addition & 2 deletions src/background/auth/implicitGrantFlow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,7 @@ async function implicitGrantFlow(
}

const data: AuthData = { access_token, ...rest } as unknown as AuthData;
// TODO: Fix IntegrationConfig types
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion -- TODO: Fix IntegrationConfig types
await setCachedAuthData(auth.id!, data);
return data;
}
Expand Down
6 changes: 0 additions & 6 deletions src/background/restrictUnauthenticatedUrlAccess.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,11 @@ describe("enforceAuthentication", () => {
});

afterEach(async () => {
// eslint-disable-next-line new-cap -- used for testing
await INTERNAL_reset();
await browser.storage.managed.clear();
await browser.storage.local.clear();
axiosMock.reset();
jest.clearAllMocks();
// eslint-disable-next-line new-cap -- used for testing
TEST_clearListeners();
});

Expand Down Expand Up @@ -123,13 +121,11 @@ describe("enforceAuthentication", () => {
expect(addListenerSpy).toHaveBeenCalledTimes(1);
expect(removeListenerSpy).toHaveBeenCalledTimes(0);

// eslint-disable-next-line new-cap -- used for testing
TEST_triggerListeners({ token: "foo" });

expect(removeListenerSpy).toHaveBeenCalledTimes(1);
addListenerSpy.mockClear();

// eslint-disable-next-line new-cap -- used for testing
TEST_triggerListeners(undefined);
await waitForEffect();
expect(addListenerSpy).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -173,7 +169,6 @@ describe("enforceAuthentication", () => {
}),
);

// eslint-disable-next-line new-cap -- used for testing
TEST_triggerListeners({ token: "foo" });

await waitForEffect();
Expand Down Expand Up @@ -218,7 +213,6 @@ describe("enforceAuthentication", () => {
// Mock that tab no longer exists
jest.mocked(browser.tabs.get).mockResolvedValue(null);

// eslint-disable-next-line new-cap -- used for testing
TEST_triggerListeners({ token: "foo" });

await waitForEffect();
Expand Down
2 changes: 1 addition & 1 deletion src/background/setToolbarIconFromTheme.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe("setToolbarIconFromTheme", () => {
// @ts-expect-error -- No need to mock the whole class for the test
global.Image = class {
src = "";
// eslint-disable-next-line no-restricted-syntax
// eslint-disable-next-line no-restricted-syntax -- This is a mock
decode = jest.fn().mockResolvedValue(undefined);
fregante marked this conversation as resolved.
Show resolved Hide resolved
};
// @ts-expect-error -- No need to mock the whole class for the test
Expand Down
1 change: 0 additions & 1 deletion src/background/telemetry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ beforeEach(async () => {
appApiMock.reset();
appApiMock.onPost("/api/events/").reply(201, {});

// eslint-disable-next-line new-cap -- test file
await TEST_flushAll();
});

Expand Down
1 change: 0 additions & 1 deletion src/bricks/effects/pageState.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { GetPageState, SetPageState } from "@/bricks/effects/pageState";
import { TEST_resetState } from "@/platform/state/stateController";

beforeEach(() => {
// eslint-disable-next-line new-cap -- test method
TEST_resetState();
});

Expand Down
1 change: 0 additions & 1 deletion src/bricks/registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ beforeEach(() => {
jest.clearAllMocks();
backgroundGetByKindsMock.mockResolvedValue([]);

// eslint-disable-next-line new-cap -- test-only method
bricksRegistry.TEST_reset();
});

Expand Down
3 changes: 0 additions & 3 deletions src/bricks/transformers/ephemeralForm/formTransformer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const showModalMock = jest.mocked(showModal);
const brick = new FormTransformer();

afterEach(async () => {
// eslint-disable-next-line new-cap -- test method
await TEST_cancelAll();
jest.clearAllMocks();
});
Expand Down Expand Up @@ -102,7 +101,6 @@ describe("FormTransformer", () => {
brickOptionsFactory(),
);

// eslint-disable-next-line new-cap -- test method
await TEST_cancelAll();

await expect(brickPromise).rejects.toThrow(CancelError);
Expand Down Expand Up @@ -134,7 +132,6 @@ describe("FormTransformer", () => {
brickOptionsFactory(),
);

// eslint-disable-next-line new-cap -- test method
await TEST_cancelAll();

await expect(brickPromise).rejects.toThrow(CancelError);
Expand Down
3 changes: 1 addition & 2 deletions src/bricks/transformers/mapping.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ export class MappingTransformer extends TransformerABC {
}

if (Object.hasOwn(mapping, key)) {
// Checking for hasOwn
// eslint-disable-next-line security/detect-object-injection
// eslint-disable-next-line security/detect-object-injection -- Checking for hasOwn
return mapping[key];
}

Expand Down
3 changes: 1 addition & 2 deletions src/bricks/transformers/parseUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ export class UrlParser extends TransformerABC {

const searchParams: Record<string, string> = {};
for (const [key, value] of parsed.searchParams.entries()) {
// Fine because value will always be a string
// eslint-disable-next-line security/detect-object-injection
// eslint-disable-next-line security/detect-object-injection -- Fine because value will always be a string
searchParams[key] = value;
}

Expand Down
6 changes: 3 additions & 3 deletions src/components/EmotionShadowRoot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
// eslint-disable-next-line no-restricted-imports -- All roads lead here
import EmotionShadowRoot from "react-shadow/emotion";

// "Every property exists" (via Proxy), TypeScript doesn't offer such type
// Also strictNullChecks config mismatch
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion
/* eslint-disable-next-line @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-unnecessary-type-assertion --
"Every property exists" (via Proxy), TypeScript doesn't offer such type
Also strictNullChecks config mismatch */
const ShadowRoot = EmotionShadowRoot.div!;

export default ShadowRoot;
2 changes: 1 addition & 1 deletion src/components/documentBuilder/documentTree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export const buildDocumentBranch: BuildDocumentBranch = (root, tracePath) => {
return componentDefinition;
};

// eslint-disable-next-line complexity
// eslint-disable-next-line complexity -- We're handling a lot of different element types
export function getComponentDefinition(
element: DocumentElement,
tracePath: DynamicPath,
Expand Down
4 changes: 2 additions & 2 deletions src/components/documentBuilder/hooks/useMoveWithinParent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,12 @@ function useMoveWithinParent(documentBodyName: string): MoveWithinParent {
const newElementsCollection = [...elementsCollection];
const toIndex = direction === "up" ? elementIndex - 1 : elementIndex + 1;

/* eslint-disable security/detect-object-injection */
/* eslint-disable security/detect-object-injection -- swapping list elements */
[newElementsCollection[elementIndex], newElementsCollection[toIndex]] = [
newElementsCollection[toIndex],
newElementsCollection[elementIndex],
];
/* eslint-enable security/detect-object-injection */
/* eslint-enable security/detect-object-injection -- re-enable */

await setElementsCollection(newElementsCollection);
setActiveElement(joinPathParts(collectionName, toIndex));
Expand Down
17 changes: 8 additions & 9 deletions src/components/documentBuilder/preview/DocumentPreview.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,43 +87,42 @@ describe("Add new element", () => {
const listElement = createNewElement("list") as ListDocumentElement;
listElement.config.element.__value__ = createNewElement("container");
const containerElement = createNewElement("container");
// There's a row in the container and a column in the row.
// eslint-disable-next-line testing-library/no-node-access
// eslint-disable-next-line testing-library/no-node-access -- There's a row in the container and a column in the row.
containerElement.children[0].children[0].children[0] = listElement;

const { container } = renderDocumentPreview(containerElement);

// Select a dropdown inside a Col in List and open it
await userEvent.click(
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: A single /* eslint-disable testing-library/no-container, testing-library/no-node-access */ might be best

container.querySelector(".col .col .addElement button"),
);
expect(
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
container.querySelector(".col .col .addElement button"),
).toHaveAttribute("aria-expanded", "true");

// Hover over the Col in the list
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
fireEvent.mouseOver(container.querySelector(".col .col"));
expect(
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
container.querySelector(".col .col .addElement button"),
).toHaveAttribute("aria-expanded", "true");

// Hover over the Container of the List, .root.root - is the Document root element
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
fireEvent.mouseOver(container.querySelector(".root.root > .container"));
expect(
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
container.querySelector(".col .col .addElement button"),
).toHaveAttribute("aria-expanded", "true");
});

test("can add an element to a container", async () => {
const { container } = renderDocumentPreview(createNewElement("container"));

// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access -- see test's TODO comment
await userEvent.click(container.querySelector(".col .addElement button"));
await userEvent.click(screen.getByText("Header", { selector: "a" }));

Expand Down
1 change: 0 additions & 1 deletion src/contentScript/lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ export function clearEditorExtension(

if (extensionPoint.kind === "actionPanel" && preserveSidebar) {
const sidebar = extensionPoint as SidebarStarterBrickABC;
// eslint-disable-next-line new-cap -- hack for action panels
sidebar.HACK_uninstallExceptExtension(extensionId);
} else {
extensionPoint.uninstall({ global: true });
Expand Down
1 change: 0 additions & 1 deletion src/contentScript/loadActivationEnhancementsCore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ describe("marketplace enhancements", () => {

afterEach(async () => {
jest.resetAllMocks();
// eslint-disable-next-line new-cap -- test method
TEST_unloadActivationEnhancements();
});

Expand Down
1 change: 0 additions & 1 deletion src/extensionConsole/pages/BrowserBanner.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { screen } from "@testing-library/react";
import { INTERNAL_reset } from "@/store/enterprise/managedStorage";

beforeEach(async () => {
// eslint-disable-next-line new-cap -- test helper method
await INTERNAL_reset();
await browser.storage.managed.clear();
});
Expand Down
2 changes: 0 additions & 2 deletions src/hooks/useFlags.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ describe("useFlags", () => {

const tokenData = tokenAuthDataFactory();

// eslint-disable-next-line new-cap
await TEST_setAuthData(tokenData);

render(
Expand All @@ -94,7 +93,6 @@ describe("useFlags", () => {
expect(appApiMock.history.get).toHaveLength(1);

// Simulate a change in auth data
// eslint-disable-next-line new-cap
TEST_triggerListeners(tokenData);

await waitForEffect();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ const serviceRegistryMock = jest.mocked(serviceRegistry);

describe("useSanitizedIntegrationConfigFormikAdapter", () => {
beforeEach(() => {
// eslint-disable-next-line new-cap -- test helper method
INTERNAL_reset();
});

Expand Down
Loading
Loading