Skip to content

Commit

Permalink
web: all aboard the anti-if bus, according to tooling (#10220)
Browse files Browse the repository at this point in the history
* web: fix esbuild issue with style sheets

Getting ESBuild, Lit, and Storybook to all agree on how to read and parse stylesheets is a serious
pain. This fix better identifies the value types (instances) being passed from various sources in
the repo to the three *different* kinds of style processors we're using (the native one, the
polyfill one, and whatever the heck Storybook does internally).

Falling back to using older CSS instantiating techniques one era at a time seems to do the trick.
It's ugly, but in the face of the aggressive styling we use to avoid Flashes of Unstyled Content
(FLoUC), it's the logic with which we're left.

In standard mode, the following warning appears on the console when running a Flow:

```
Autofocus processing was blocked because a document already has a focused element.
```

In compatibility mode, the following **error** appears on the console when running a Flow:

```
crawler-inject.js:1106 Uncaught TypeError: Failed to execute 'observe' on 'MutationObserver': parameter 1 is not of type 'Node'.
    at initDomMutationObservers (crawler-inject.js:1106:18)
    at crawler-inject.js:1114:24
    at Array.forEach (<anonymous>)
    at initDomMutationObservers (crawler-inject.js:1114:10)
    at crawler-inject.js:1549:1
initDomMutationObservers @ crawler-inject.js:1106
(anonymous) @ crawler-inject.js:1114
initDomMutationObservers @ crawler-inject.js:1114
(anonymous) @ crawler-inject.js:1549
```

Despite this error, nothing seems to be broken and flows work as anticipated.

* web: all-aboard the anti-if bus, according to tooling

This commit revises a number of bugs `eslint` has been complaining about for awhile now. This is the
lesser of two PRs that will address this issue, and in this case the two biggest problems were
inappropriate conditionals (using a `switch` for a single comparison), unnecessarily named returns,
empty returns. This brings our use of conditions in-line with the coding standards we _say_ we want
in eslintrc!

* web: better names and logic for comparing the dates of Xliff vs generated files

* Missed one.

* Fixed a redirect issue that was creating an empty file in the ./web folder
  • Loading branch information
kensternberg-authentik authored Jul 15, 2024
1 parent c0063c1 commit 085ab3c
Show file tree
Hide file tree
Showing 13 changed files with 114 additions and 80 deletions.
23 changes: 12 additions & 11 deletions web/scripts/build-locales.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,39 +5,38 @@ import process from "process";

const localizeRules = JSON.parse(fs.readFileSync("./lit-localize.json", "utf-8"));

function compareXlfAndSrc(loc) {
const xlf = path.join("./xliff", `${loc}.xlf`);
const src = path.join("./src/locales", `${loc}.ts`);
function generatedFileIsUpToDateWithXliffSource(loc) {
const xliff = path.join("./xliff", `${loc}.xlf`);
const gened = path.join("./src/locales", `${loc}.ts`);

// Returns false if: the expected XLF file doesn't exist, The expected
// generated file doesn't exist, or the XLF file is newer (has a higher date)
// than the generated file. The missing XLF file is important enough it
// generates a unique error message and halts the build.

try {
var xlfStat = fs.statSync(xlf);
var xlfStat = fs.statSync(xliff);
} catch (_error) {
console.error(`lit-localize expected '${loc}.xlf', but XLF file is not present`);
process.exit(1);
}

// If the generated file doesn't exist, of course it's not up to date.
try {
var srcStat = fs.statSync(src);
var genedStat = fs.statSync(gened);
} catch (_error) {
return false;
}

// if the xlf is newer (greater) than src, it's out of date.
if (xlfStat.mtimeMs > srcStat.mtimeMs) {
return false;
}
return true;
// if the generated file is the same age or older (date is greater) than the xliff file, it's
// presumed to have been generated by that file and is up-to-date.
return genedStat.mtimeMs >= xlfStat.mtimeMs;
}

// For all the expected files, find out if any aren't up-to-date.

const upToDate = localizeRules.targetLocales.reduce(
(acc, loc) => acc && compareXlfAndSrc(loc),
(acc, loc) => acc && generatedFileIsUpToDateWithXliffSource(loc),
true,
);

Expand All @@ -61,7 +60,9 @@ if (!upToDate) {
.map((locale) => `Locale '${locale}' has ${counts.get(locale)} missing translations`)
.join("\n");

// eslint-disable-next-line no-console
console.log(`Translation tables rebuilt.\n${report}\n`);
}

// eslint-disable-next-line no-console
console.log("Locale ./src is up-to-date");
1 change: 1 addition & 0 deletions web/scripts/check-spelling.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ const cmd = [
"-S './src/locales/**' ./src -s",
].join(" ");

// eslint-disable-next-line no-console
console.log(execSync(cmd, { encoding: "utf8" }));
55 changes: 55 additions & 0 deletions web/scripts/eslint-nightmare.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { execFileSync } from "child_process";
import { ESLint } from "eslint";
import path from "path";
import process from "process";

// Code assumes this script is in the './web/scripts' folder.
const projectRoot = execFileSync("git", ["rev-parse", "--show-toplevel"], {
encoding: "utf8",
}).replace("\n", "");
process.chdir(path.join(projectRoot, "./web"));

const eslintConfig = {
overrideConfig: {
env: {
browser: true,
es2021: true,
},
extends: [
"eslint:recommended",
"plugin:@typescript-eslint/recommended",
"plugin:lit/recommended",
"plugin:custom-elements/recommended",
"plugin:storybook/recommended",
"plugin:sonarjs/recommended",
],
parser: "@typescript-eslint/parser",
parserOptions: {
ecmaVersion: 12,
sourceType: "module",
},
plugins: ["@typescript-eslint", "lit", "custom-elements", "sonarjs"],
rules: {
"indent": "off",
"linebreak-style": ["error", "unix"],
"quotes": ["error", "double", { avoidEscape: true }],
"semi": ["error", "always"],
"@typescript-eslint/ban-ts-comment": "off",
"sonarjs/cognitive-complexity": ["error", 9],
"sonarjs/no-duplicate-string": "off",
"sonarjs/no-nested-template-literals": "off",
},
},
};

const updated = ["./src/", "./build.mjs", "./scripts/*.mjs"];

const eslint = new ESLint(eslintConfig);
const results = await eslint.lintFiles(updated);
const formatter = await eslint.loadFormatter("stylish");
const resultText = formatter.format(results);
const errors = results.reduce((acc, result) => acc + result.errorCount, 0);

// eslint-disable-next-line no-console
console.log(resultText);
process.exit(errors > 1 ? 1 : 0);
2 changes: 1 addition & 1 deletion web/scripts/patch-spotlight.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

TARGET="./node_modules/@spotlightjs/overlay/dist/index-"[0-9a-f]*.js

if [[ $(grep -L "QX2" "$TARGET" > /dev/null 2&>1) ]]; then
if [[ $(grep -L "QX2" "$TARGET" > /dev/null 2> /dev/null) ]]; then
patch --forward -V none --no-backup-if-mismatch -p0 $TARGET <<EOF
TARGET=$(find "./node_modules/@spotlightjs/overlay/dist/" -name "index-[0-9a-f]*.js");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,7 @@ export class ApplicationWizardCommitApplication extends BasePanel {
);
if (!providerModel) {
throw new Error(
`Could not determine provider model from user request: ${JSON.stringify(
this.wizard,
null,
2,
)}`,
`Could not determine provider model from user request: ${JSON.stringify(this.wizard, null, 2)}`,
);
}

Expand All @@ -118,7 +114,6 @@ export class ApplicationWizardCommitApplication extends BasePanel {
};

this.send(request);
return;
}
}

Expand Down
15 changes: 6 additions & 9 deletions web/src/admin/property-mappings/PropertyMappingTestForm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import "@goauthentik/elements/forms/SearchSelect";
import YAML from "yaml";

import { msg } from "@lit/localize";
import { TemplateResult, html } from "lit";
import { TemplateResult, html, nothing } from "lit";
import { customElement, property } from "lit/decorators.js";
import { ifDefined } from "lit/directives/if-defined.js";

Expand Down Expand Up @@ -66,14 +66,11 @@ export class PolicyTestForm extends Form<PropertyMappingTestRequest> {
</ak-form-element-horizontal>`;
}

renderExampleButtons(): TemplateResult {
const header = html`<p>${msg("Example context data")}</p>`;
switch (this.mapping?.metaModelName) {
case "authentik_sources_ldap.ldappropertymapping":
return html`${header}${this.renderExampleLDAP()}`;
default:
return html``;
}
renderExampleButtons() {
return this.mapping?.metaModelName === "authentik_sources_ldap.ldappropertymapping"
? html`<p>${msg("Example context data")}</p>
${this.renderExampleLDAP()}`
: nothing;
}

renderExampleLDAP(): TemplateResult {
Expand Down
14 changes: 6 additions & 8 deletions web/src/common/helpers/webauthn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,11 @@ export function transformCredentialCreateOptions(
user.id = u8arr(b64enc(u8arr(stringId)));
const challenge = u8arr(credentialCreateOptions.challenge.toString());

const transformedCredentialCreateOptions = Object.assign({}, credentialCreateOptions, {
return {
...credentialCreateOptions,
challenge,
user,
});

return transformedCredentialCreateOptions;
};
}

export interface Assertion {
Expand Down Expand Up @@ -98,12 +97,11 @@ export function transformCredentialRequestOptions(
},
);

const transformedCredentialRequestOptions = Object.assign({}, credentialRequestOptions, {
return {
...credentialRequestOptions,
challenge,
allowCredentials,
});

return transformedCredentialRequestOptions;
};
}

export interface AuthAssertion {
Expand Down
12 changes: 7 additions & 5 deletions web/src/elements/Tabs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,13 @@ export class Tabs extends AKElement {
const pages = Array.from(this.querySelectorAll(":scope > [slot^='page-']"));
if (window.location.hash.includes(ROUTE_SEPARATOR)) {
const params = getURLParams();
if (this.pageIdentifier in params && !this.currentPage) {
if (this.querySelector(`[slot='${params[this.pageIdentifier]}']`) !== null) {
// To update the URL to match with the current slot
this.onClick(params[this.pageIdentifier] as string);
}
if (
this.pageIdentifier in params &&
!this.currentPage &&
this.querySelector(`[slot='${params[this.pageIdentifier]}']`) !== null
) {
// To update the URL to match with the current slot
this.onClick(params[this.pageIdentifier] as string);
}
}
if (!this.currentPage) {
Expand Down
11 changes: 7 additions & 4 deletions web/src/elements/forms/SearchSelect/ak-search-select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,13 @@ export class SearchSelect<T> extends CustomEmitterElement(AKElement) {
const onFocus = (ev: FocusEvent) => {
this.open = true;
this.renderMenu();
if (this.blankable && this.renderedValue === this.emptyOption) {
if (ev.target && ev.target instanceof HTMLInputElement) {
ev.target.value = "";
}
if (
this.blankable &&
this.renderedValue === this.emptyOption &&
ev.target &&
ev.target instanceof HTMLInputElement
) {
ev.target.value = "";
}
};

Expand Down
18 changes: 5 additions & 13 deletions web/src/elements/sidebar/SidebarItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,19 +117,11 @@ export class SidebarItem extends AKElement {
if (!this.path) {
return false;
}
if (this.path) {
const ourPath = this.path.split(";")[0];
if (new RegExp(`^${ourPath}$`).exec(path)) {
return true;
}
}
return this.activeMatchers.some((v) => {
const match = v.exec(path);
if (match !== null) {
return true;
}
return false;
});

const ourPath = this.path.split(";")[0];
const pathIsWholePath = new RegExp(`^${ourPath}$`).test(path);
const pathIsAnActivePath = this.activeMatchers.some((v) => v.test(path));
return pathIsWholePath || pathIsAnActivePath;
}

expandParentRecursive(activePath: string, item: SidebarItem): void {
Expand Down
7 changes: 2 additions & 5 deletions web/src/flow/stages/prompt/PromptStage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,11 @@ ${prompt.initialValue}</textarea

shouldRenderInWrapper(prompt: StagePrompt): boolean {
// Special types that aren't rendered in a wrapper
if (
return !(
prompt.type === PromptTypeEnum.Static ||
prompt.type === PromptTypeEnum.Hidden ||
prompt.type === PromptTypeEnum.Separator
) {
return false;
}
return true;
);
}

renderField(prompt: StagePrompt): TemplateResult {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ export class UserSettingsFlowExecutor
})
.then((data) => {
this.challenge = data;
if (this.challenge.responseErrors) {
return false;
}
return true;
return !this.challenge.responseErrors;
})
.catch((e: Error | ResponseError) => {
this.errorMessage(e);
Expand Down
24 changes: 10 additions & 14 deletions web/src/user/user-settings/details/stages/prompt/PromptStage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,16 @@ import { PromptTypeEnum, StagePrompt } from "@goauthentik/api";
@customElement("ak-user-stage-prompt")
export class UserSettingsPromptStage extends PromptStage {
renderPromptInner(prompt: StagePrompt): TemplateResult {
switch (prompt.type) {
// Checkbox requires slightly different rendering here due to the use of horizontal form elements
case PromptTypeEnum.Checkbox:
return html`<input
type="checkbox"
class="pf-c-check__input"
name="${prompt.fieldKey}"
?checked=${prompt.initialValue !== ""}
?required=${prompt.required}
style="vertical-align: bottom"
/>`;
default:
return super.renderPromptInner(prompt);
}
return prompt.type === PromptTypeEnum.Checkbox
? html`<input
type="checkbox"
class="pf-c-check__input"
name="${prompt.fieldKey}"
?checked=${prompt.initialValue !== ""}
?required=${prompt.required}
style="vertical-align: bottom"
/>`
: super.renderPromptInner(prompt);
}

renderField(prompt: StagePrompt): TemplateResult {
Expand Down

0 comments on commit 085ab3c

Please sign in to comment.