Skip to content

Commit

Permalink
Updates to address changes in ESLint etc
Browse files Browse the repository at this point in the history
- TEMPORARY: warn on `throw` with non-object value. I know we recently decided not to leave warnings unaddressed, but we will likely address the affected cases in upcoming work on form load error conditions (in context of external secondary instances)

- Flat config is no longer experimental in VSCode

- FlatCompat is evidently no longer necessary for Vue language support (and we drop @eslint/eslintrc as a direct dependency, since FlatCompat was all we used it for)

- Address a few changes in typescript-eslint specifically:

  - Some rules have been split up. To the extent I’m aware of and understand these changes, any affected overrides are updated to match current behavior (or as close as possible).

  - Some rules have gotten more strict. The one which stands out the most is detection of unused variables. Previously, no error was produced if a type is derived from variables with no runtime use. This is no longer the case.

    One case is ignored, and the reasoning for it is already explained in a JSDoc block.

    One case that may not be obvious is replacing a `ui-solid` derived component type with Solid’s provided `Component` type which is specifically intended for the use case. (The usage is a bit odd, it’s overriding the props type of a styled component from the SUID component library.)

    The remaining cases are calls to initialize Vitest assertion extensions. These calls cause global side effects, assigning custom assertion methods on the `expect()` result object. We then derive the types from those calls’ return values, and assign them to the corresponding types for the same extended `expect()` result object. I’ve chosen to export these cases (as that treats them as used), but could consider explicitly ignoring them instead. I think this pattern is probably more flexible for the future (i.e. if we want to put some effort into converting our assertion extensions into something less global side-effect-y).

  - A new rule which seems sensible: it’s recommended to use `RegExp#exec` rather than `String#match` for regular expressions without the global (`g`) flag.

  - As noted in a few override comments: a very sensible error is now produced for statements which don’t appear to do anything. We override this in cases where we have known logic triggering reactive subscriptions by reading a reactive getter property.

    One other affected case that may not be obvious: a pair of assertions in an `xpath` test were mistakenly separated by commas. This behaved correctly, but would be misleading in terms of intent if left as-is. I am almost certain it was a typo, introduced while rapidly porting tests from openrosa-xpath-evaluator to our then-new XPath implementation.

  - tseslint improves detecting usage patterns that might break `this` binding for object methods (e.g. by destructuring or otherwise taking a direct reference to them). We largely accommodate this by converting newly caught cases to arrow functions. In some cases, we also convert the type definition for such methods to arrow function properties.
  • Loading branch information
eyelidlessness committed Sep 5, 2024
1 parent 69e5263 commit bf9f8c3
Show file tree
Hide file tree
Showing 27 changed files with 67 additions and 102 deletions.
10 changes: 5 additions & 5 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

// Formatting
"eslint.enable": true,
"eslint.experimental.useFlatConfig": true,
"eslint.useFlatConfig": true,
"eslint.format.enable": true,
"eslint.nodePath": "../node_modules/.bin",
"prettier.configPath": "prettier.config.js",
Expand All @@ -28,11 +28,11 @@
"editor.defaultFormatter": "dbaeumer.vscode-eslint",
"editor.formatOnSave": true
},
"[xml]": {
"editor.defaultFormatter": "redhat.vscode-xml"
},
// This corresponds to the `lib` we're using everywhere. Setting it here
// helps to address editor-only type errors in tooling files, where VSCode
// doesn't pick up the top-level `tsconfig.tools.json`.
"js/ts.implicitProjectConfig.target": "ES2022",
"[xml]": {
"editor.defaultFormatter": "redhat.vscode-xml"
}
"js/ts.implicitProjectConfig.target": "ES2022"
}
60 changes: 19 additions & 41 deletions eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
// annotating with a date, but it'd be nice if we could just derive that from
// git history or whatever.

/// <reference path="./vendor-types/@eslint/eslintrc.d.ts" />
/// <reference path="./vendor-types/@vue/eslint-config-typescript/index.d.ts" />
/// <reference path="./vendor-types/eslint-plugin-no-only-tests.d.ts" />
/// <reference path="./vendor-types/eslint-plugin-vue/lib/configs/base.d.ts" />
Expand All @@ -18,7 +17,6 @@
/// <reference path="./vendor-types/eslint-plugin-vue/lib/index.d.ts" />
/// <reference path="./vendor-types/eslint-plugin-vue/lib/processor.d.ts" />

import { FlatCompat } from '@eslint/eslintrc';
import eslint from '@eslint/js';
import eslintConfigPrettier from 'eslint-config-prettier';
import jsdoc from 'eslint-plugin-jsdoc';
Expand All @@ -30,19 +28,8 @@ import vue3Recommended from 'eslint-plugin-vue/lib/configs/vue3-recommended.js';
import vue3StronglyRecommended from 'eslint-plugin-vue/lib/configs/vue3-strongly-recommended.js';
import vueProcessor from 'eslint-plugin-vue/lib/processor.js';
import { builtinModules } from 'node:module';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import tseslint from 'typescript-eslint';

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

const compat = new FlatCompat({
baseDirectory: __dirname,
resolvePluginsRelativeTo: __dirname,
recommendedConfig: eslint.configs.recommended,
allConfig: eslint.configs.all,
});
import vueESLintParser from 'vue-eslint-parser';

/**
* @param {string} pathSansExtension
Expand Down Expand Up @@ -147,14 +134,11 @@ export default tseslint.config(
* Note: Contrary to the defaults provided in a Vue template project, it
* was found that linting applies most consistently by applying
* Vue-specific parsing **after** TypeScript.
*
* TODO: after breaking up the parser and rule aspects, it seemed likely
* that we could stop using `FlatCompat` for this. For reasons that are
* unclear, I wasn't able to find a way to get that working.
*/
...compat
.config({
parser: 'vue-eslint-parser',
{
files: [vuePackageGlob],
languageOptions: {
parser: vueESLintParser,
parserOptions: {
/**
* @see {@link https://github.com/vuejs/vue-eslint-parser/issues/173#issuecomment-1367298274}
Expand All @@ -172,17 +156,13 @@ export default tseslint.config(
'<template>': 'espree',
},
},
})
.map((vueConfig) => ({
files: [vuePackageGlob],
plugins: {
vue: vuePlugin,
'@typescript-eslint': tseslint.plugin,
},
processor: vueProcessor,

...vueConfig,
})),
},
plugins: {
vue: vuePlugin,
'@typescript-eslint': tseslint.plugin,
},
processor: vueProcessor,
},

eslintConfigPrettier,
],
Expand Down Expand Up @@ -325,6 +305,7 @@ export default tseslint.config(
'@typescript-eslint/no-unsafe-call': 'error',
'@typescript-eslint/no-unsafe-member-access': 'error',
'@typescript-eslint/no-unsafe-return': 'error',
'@typescript-eslint/only-throw-error': 'warn',
'@typescript-eslint/prefer-nullish-coalescing': 'error',
'@typescript-eslint/prefer-optional-chain': 'error',
'@typescript-eslint/prefer-string-starts-ends-with': 'error',
Expand All @@ -341,6 +322,12 @@ export default tseslint.config(
allowSingleExtends: true,
},
],
'@typescript-eslint/no-empty-object-type': [
'error',
{
allowInterfaces: 'with-single-extends',
},
],
'prefer-const': 'error',

// Ensure Node built-ins aren't used by default
Expand Down Expand Up @@ -399,15 +386,6 @@ export default tseslint.config(
'packages/xforms-engine/vite.*.config.ts',
'packages/*/tools/**/*',
'packages/tree-sitter-xpath/scripts/build/*.mjs',

// TODO: in theory, all e2e tests (if they continue to be run with
// Playwright) are technically run in a "Node" environment, although
// they will likely exercise non-Node code when calling into the
// Playwright-managed browser process. I'm adding this special case
// mainly to make note of this because it's unclear what the best
// solution will be for mixed Node-/browser-API code in terms of type
// safety and linting.
'packages/tree-sitter-xpath/e2e/sub-expression-queries.test.ts',
],
rules: {
'no-restricted-imports': 'off',
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
"dependencies": {
"@changesets/changelog-github": "^0.5.0",
"@changesets/cli": "^2.27.8",
"@eslint/eslintrc": "^3.1.0",
"@eslint/js": "^9.9.1",
"@tsconfig/node20": "^20.1.4",
"@types/eslint": "^9.6.1",
Expand Down
3 changes: 2 additions & 1 deletion packages/common/src/test/assertions/typeofAssertion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { AnyConstructor, AnyFunction } from '../../../types/helpers.d.ts';
/**
* @see {@link Typeof}
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- this is purposefully derived, as described in `Typeof` JSDoc.
const TYPEOF = typeof ('' as unknown);

/**
Expand Down Expand Up @@ -31,7 +32,7 @@ type Typeof = typeof TYPEOF;
* this more expanded type, but in many cases it would fail to do so if we only
* specify {@link Function}.
*/
// eslint-disable-next-line @typescript-eslint/ban-types
// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type
type TypeofFunction = AnyConstructor | AnyFunction | Function;

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/assertion/extensions/answers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const matchDefaultMessage = (condition: ValidationCondition) => {
};
};

const answerExtensions = extendExpect(expect, {
export const answerExtensions = extendExpect(expect, {
toEqualAnswer: new SymmetricTypedExpectExtension(assertComparableAnswer, (actual, expected) => {
const pass = actual.stringValue === expected.stringValue;

Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/assertion/extensions/appearances.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const hasAppearance = (node: AnyNode, appearance: string): boolean => {
return node.appearances?.[appearance] === true;
};

const appearanceExtensions = extendExpect(expect, {
export const appearanceExtensions = extendExpect(expect, {
toHaveAppearance: new AsymmetricTypedExpectExtension(
assertEngineNode,
assertString,
Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/assertion/extensions/body-classes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const hasClass = (node: RootNode, className: string): boolean => {
return node.classes?.[className] === true;
};

const bodyClassesExtensions = extendExpect(expect, {
export const bodyClassesExtensions = extendExpect(expect, {
toHaveClass: new AsymmetricTypedExpectExtension(
assertRootNode,
assertString,
Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/assertion/extensions/choices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const getContainsChoicesResult = (
return error == null || error;
};

const choiceExtensions = extendExpect(expect, {
export const choiceExtensions = extendExpect(expect, {
toContainChoices: new AsymmetricTypedExpectExtension(
assertSelectList,
assertExpectedChoicesArray,
Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/assertion/extensions/form-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ type AssertJRFormDef = (value: unknown) => asserts value is JRFormDef;

const assertJRFormDef: AssertJRFormDef = (value) => assertInstanceType(JRFormDef, value);

const formStateExtensions = extendExpect(expect, {
export const formStateExtensions = extendExpect(expect, {
/**
* **PORTING NOTES**
*
Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/assertion/extensions/node-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
import { expect } from 'vitest';
import { assertEngineNode } from './shared-type-assertions.ts';

const nodeStateExtensions = extendExpect(expect, {
export const nodeStateExtensions = extendExpect(expect, {
toBeRelevant: new StaticConditionExpectExtension(assertEngineNode, {
currentState: { relevant: true },
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { JRTreeReference } from '../../jr/xpath/JRTreeReference.ts';

const assertJRTreeReference = instanceAssertion(JRTreeReference);

const treeReferenceExtensions = extendExpect(expect, {
export const treeReferenceExtensions = extendExpect(expect, {
/**
* **PORTING NOTES**
*
Expand Down
2 changes: 1 addition & 1 deletion packages/scenario/src/jr/Scenario.ts
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ export class Scenario {
}

const [, positionPredicatedReference, positionExpression] =
reference.match(/^(.*\/[^/[]+)\[(\d+)\]\/[^[]+$/) ?? [];
/^(.*\/[^/[]+)\[(\d+)\]\/[^[]+$/.exec(reference) ?? [];

if (positionPredicatedReference == null || positionExpression == null) {
return;
Expand Down
1 change: 1 addition & 0 deletions packages/ui-solid/src/components/XForm/XFormDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const serializeNode = (node: AnyNode, depth = 0): string => {

if (children == null) {
// Just read it to make it reactive...
// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
currentState.value;

const serializedLeafNode = `<${nodeName}>${escapeXMLText((node as FakeSerializationInterface).contextNode.textContent ?? '')}</${nodeName}>`;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Box, styled } from '@suid/material';
import type { JSX } from 'solid-js';
import type { Component, JSX } from 'solid-js';
import { Show } from 'solid-js';

interface XFormRelevanceGuardProps {
Expand All @@ -8,9 +8,7 @@ interface XFormRelevanceGuardProps {
readonly children: JSX.Element;
}

const BaseXFormRelevanceGuardBox = (props: XFormRelevanceGuardProps) => <Box>{props.children}</Box>;

const XFormRelevanceGuardBox = styled<typeof BaseXFormRelevanceGuardBox>(Box)(({
const XFormRelevanceGuardBox = styled<Component<XFormRelevanceGuardProps>>(Box)(({
props,
theme,
}) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { computed } from 'vue';
const props = defineProps<{ appearances: SelectNodeAppearances}>();
const nColumnstyle = computed(() => {
const numberOfColumns = [...props.appearances].find(a => a.match(/columns-\d+/))?.match(/\d+/)?.[0];
const numberOfColumns = [...props.appearances].find(a => /columns-\d+/.exec(a))?.match(/\d+/)?.[0];
return numberOfColumns ? `grid-template-columns: repeat(${numberOfColumns}, 1fr);` : '';
});
</script>
Expand All @@ -23,7 +23,7 @@ const nColumnstyle = computed(() => {
display: flex;
flex-wrap: wrap;
gap: 10px 20px;
}
.columns {
Expand All @@ -41,4 +41,4 @@ const nColumnstyle = computed(() => {
grid-template-columns: repeat(5, 1fr);
}
}
</style>
</style>
4 changes: 1 addition & 3 deletions packages/xforms-engine/src/XFormDataType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ export type UnsupportedDataType = typeof UNSUPPORTED_DATA_TYPE;
/**
* Like JavaRosa. Presumably for e.g. groups with explicit binds (`relevant` etc)?
*/
const NULL_DATA_TYPE = 'NULL';

export type NullDataType = typeof NULL_DATA_TYPE;
export type NullDataType = 'NULL';

/**
* As in ODK XForms Spec.
Expand Down
4 changes: 4 additions & 0 deletions packages/xforms-engine/src/instance/Root.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ export class Root
override subscribe(): void {
super.subscribe();

// TODO: typescript-eslint is right to object to this! We should _at least_
// make internal reactive reads obvious, i.e. function calls.
//
// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
this.engineState.activeLanguage;
}
}
8 changes: 8 additions & 0 deletions packages/xforms-engine/src/instance/abstract/InstanceNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,9 +246,17 @@ export abstract class InstanceNode<
// subscriptions would be established by evaluation of the expressions
// themselves (as they traverse instance state and access values), rather
// than this safer/less focused approach.

// TODO: typescript-eslint is right to object to these! We should _at least_
// make internal reactive reads obvious, i.e. function calls.

// eslint-disable-next-line @typescript-eslint/no-unused-expressions
engineState.reference;
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
engineState.relevant;
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
engineState.children;
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
engineState.value;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export type ComputedPropertySpec<T> = Accessor<T>;
/**
* @see {@link StaticPropertySpec}
*/
// eslint-disable-next-line @typescript-eslint/ban-types, @typescript-eslint/no-explicit-any
// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type, @typescript-eslint/no-explicit-any
type NonStaticValue = Function | Signal<any> | SimpleAtomicState<any>;

/**
Expand Down
5 changes: 5 additions & 0 deletions packages/xforms-engine/test/helpers/reactive/internal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ describe('Internal minimal reactivity implementation (itself currently for use i
effect(() => {
effectReactions += 1;

// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
object.a;
});

Expand All @@ -57,6 +58,7 @@ describe('Internal minimal reactivity implementation (itself currently for use i
effect(() => {
effectReactions += 1;

// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
object.a;
});

Expand Down Expand Up @@ -89,12 +91,14 @@ describe('Internal minimal reactivity implementation (itself currently for use i
effect(() => {
effectReactions.a += 1;

// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
object.a;
});

effect(() => {
effectReactions.b += 1;

// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
object.b;
});

Expand Down Expand Up @@ -186,6 +190,7 @@ describe('Internal minimal reactivity implementation (itself currently for use i
effect(() => {
effectReactions += 1;

// eslint-disable-next-line @typescript-eslint/no-unused-expressions -- read == subscribe
object.b;
});

Expand Down
2 changes: 1 addition & 1 deletion packages/xpath/src/functions/xforms/datetime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ export const date = new FunctionImplementation(
}

if (!DATE_OR_DATE_TIME_PATTERN.test(string)) {
const unpaddedMatches = string.match(UNPADDED_MONTH_DAY_PATTERN);
const unpaddedMatches = UNPADDED_MONTH_DAY_PATTERN.exec(string);

if (unpaddedMatches == null) {
return new DateTimeLikeEvaluation(context, null);
Expand Down
4 changes: 0 additions & 4 deletions packages/xpath/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,10 +266,6 @@ export class TestContext<XForms extends boolean = false> {

expect(stringValue, message).toMatch(pattern);
}

assertThrows(): never {
throw '';
}
}

export const createTestContext = (xml?: string, options: TestContextOptions = {}): TestContext => {
Expand Down
Loading

0 comments on commit bf9f8c3

Please sign in to comment.