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

fix(editor): Fix External secrets typecheck (no-changelog) #9434

Merged
merged 6 commits into from
May 17, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
17 changes: 10 additions & 7 deletions packages/editor-ui/src/Interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1800,18 +1800,19 @@ export interface ExternalSecretsProviderSecret {

export type ExternalSecretsProviderData = Record<string, IUpdateInformation['value']>;

export type ExternalSecretsProviderProperty = INodeProperties;

export type ExternalSecretsProviderState = 'connected' | 'tested' | 'initializing' | 'error';

export interface ExternalSecretsProvider {
icon: string;
name: string;
displayName: string;
connected: boolean;
connectedAt: string | false;
state: 'connected' | 'tested' | 'initializing' | 'error';
state: ExternalSecretsProviderState;
data?: ExternalSecretsProviderData;
}

export interface ExternalSecretsProviderWithProperties extends ExternalSecretsProvider {
properties: INodeProperties[];
properties?: ExternalSecretsProviderProperty[];
}

export type CloudUpdateLinkSourceType =
Expand All @@ -1831,7 +1832,8 @@ export type CloudUpdateLinkSourceType =
| 'variables'
| 'community-nodes'
| 'workflow-history'
| 'worker-view';
| 'worker-view'
| 'external-secrets';

export type UTMCampaign =
| 'upgrade-custom-data-filter'
Expand All @@ -1850,7 +1852,8 @@ export type UTMCampaign =
| 'upgrade-community-nodes'
| 'upgrade-workflow-history'
| 'upgrade-advanced-permissions'
| 'upgrade-worker-view';
| 'upgrade-worker-view'
| 'upgrade-external-secrets';

export type N8nBanners = {
[key in BannerName]: {
Expand Down
8 changes: 2 additions & 6 deletions packages/editor-ui/src/api/externalSecrets.ee.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
import type {
IRestApiContext,
ExternalSecretsProvider,
ExternalSecretsProviderWithProperties,
} from '@/Interface';
import type { IRestApiContext, ExternalSecretsProvider } from '@/Interface';
import { makeRestApiRequest } from '@/utils/apiUtils';

export const getExternalSecrets = async (
Expand All @@ -20,7 +16,7 @@ export const getExternalSecretsProviders = async (
export const getExternalSecretsProvider = async (
context: IRestApiContext,
id: string,
): Promise<ExternalSecretsProviderWithProperties> => {
): Promise<ExternalSecretsProvider> => {
return await makeRestApiRequest(context, 'GET', `/external-secrets/providers/${id}`);
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script lang="ts" setup>
import type { PropType, Ref } from 'vue';
import type { PropType } from 'vue';
import type { ExternalSecretsProvider } from '@/Interface';
import ExternalSecretsProviderImage from '@/components/ExternalSecretsProviderImage.ee.vue';
import ExternalSecretsProviderConnectionSwitch from '@/components/ExternalSecretsProviderConnectionSwitch.ee.vue';
Expand All @@ -10,7 +10,8 @@ import { useI18n } from '@/composables/useI18n';
import { useExternalSecretsProvider } from '@/composables/useExternalSecretsProvider';
import { EXTERNAL_SECRETS_PROVIDER_MODAL_KEY } from '@/constants';
import { DateTime } from 'luxon';
import { computed, nextTick, onMounted, toRefs } from 'vue';
import { computed, nextTick, onMounted, toRef } from 'vue';
import { isDateObject } from '@/utils/typeGuards';

const props = defineProps({
provider: {
Expand All @@ -24,15 +25,12 @@ const i18n = useI18n();
const uiStore = useUIStore();
const toast = useToast();

const { provider } = toRefs(props) as Ref<ExternalSecretsProvider>;
const providerData = computed(() => provider.value.data);
const {
connectionState,
initialConnectionState,
normalizedProviderData,
testConnection,
setConnectionState,
} = useExternalSecretsProvider(provider, providerData);
const provider = toRef(props, 'provider');
const providerData = computed(() => provider.value.data ?? {});
const { connectionState, testConnection, setConnectionState } = useExternalSecretsProvider(
provider,
providerData,
);

const actionDropdownOptions = computed(() => [
{
Expand All @@ -50,11 +48,15 @@ const actionDropdownOptions = computed(() => [
]);

const canConnect = computed(() => {
return props.provider.connected || Object.keys(props.provider.data).length > 0;
return props.provider.connected || Object.keys(providerData.value).length > 0;
});

const formattedDate = computed((provider: ExternalSecretsProvider) => {
return DateTime.fromISO(props.provider.connectedAt ?? new Date()).toFormat('dd LLL yyyy');
const formattedDate = computed(() => {
return DateTime.fromISO(
isDateObject(provider.value.connectedAt)
? provider.value.connectedAt.toISOString()
: provider.value.connectedAt || new Date().toISOString(),
).toFormat('dd LLL yyyy');
});

onMounted(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
<script lang="ts" setup>
/* eslint-disable @typescript-eslint/ban-ts-comment */
import type { PropType } from 'vue';
import type { ExternalSecretsProvider } from '@/Interface';
import { computed } from 'vue';

// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

expect-error does not feel right here. Could we follow something similar to here declare module '*.svg'? https://stackoverflow.com/questions/52759220/importing-images-in-typescript-react-cannot-find-module

Though in my quick testing, it did not work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that too. For some reason vue-tsc doesn't pick up the shims file. 🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a TODO comment here?

import infisical from '../assets/images/infisical.webp';
Copy link
Contributor

Choose a reason for hiding this comment

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

you can put a module definition to one of the shim.d.ts files
declare module '*.webp'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, it doesn't get picked up by vue-tsc. 🥲

// @ts-expect-error
import doppler from '../assets/images/doppler.webp';
// @ts-expect-error
import vault from '../assets/images/hashicorp.webp';
// @ts-expect-error
import awsSecretsManager from '../assets/images/aws-secrets-manager.svg';

const props = defineProps({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import Modal from './Modal.vue';
import { EXTERNAL_SECRETS_PROVIDER_MODAL_KEY, MODAL_CONFIRM } from '@/constants';
import { computed, onMounted, ref } from 'vue';
import type { PropType, Ref } from 'vue';

Check failure on line 5 in packages/editor-ui/src/components/ExternalSecretsProviderModal.ee.vue

View workflow job for this annotation

GitHub Actions / Lint changes

'Ref' is defined but never used
import type { EventBus } from 'n8n-design-system/utils';
import { useExternalSecretsProvider } from '@/composables/useExternalSecretsProvider';
import { useI18n } from '@/composables/useI18n';
Expand All @@ -10,7 +10,6 @@
import { useToast } from '@/composables/useToast';
import { useExternalSecretsStore } from '@/stores/externalSecrets.ee.store';
import { useUIStore } from '@/stores/ui.store';
import { useRoute } from 'vue-router';
import ParameterInputExpanded from '@/components/ParameterInputExpanded.vue';
import type {
IUpdateInformation,
Expand All @@ -29,7 +28,7 @@
},
});

const defaultProviderData = {
const defaultProviderData: Record<string, Partial<ExternalSecretsProviderData>> = {
infisical: {
siteURL: 'https://app.infisical.com',
},
Expand All @@ -39,7 +38,6 @@
const uiStore = useUIStore();
const toast = useToast();
const i18n = useI18n();
const route = useRoute();
const { confirm } = useMessage();

const saving = ref(false);
Expand All @@ -50,7 +48,7 @@

const provider = computed<ExternalSecretsProvider | undefined>(() =>
externalSecretsStore.providers.find((p) => p.name === props.data.name),
) as Ref<ExternalSecretsProvider>;
);
const providerData = ref<ExternalSecretsProviderData>({});
const {
connectionState,
Expand All @@ -64,15 +62,15 @@
const providerDataUpdated = computed(() => {
return Object.keys(providerData.value).find((key) => {
const value = providerData.value[key];
const originalValue = provider.value.data[key];
const originalValue = provider.value?.data?.[key];

return value !== originalValue;
});
});

const canSave = computed(
() =>
provider.value.properties
provider.value?.properties
?.filter((property) => property.required && shouldDisplayProperty(property))
.every((property) => {
const value = providerData.value[property.name];
Expand All @@ -82,21 +80,22 @@

onMounted(async () => {
try {
const provider = await externalSecretsStore.getProvider(props.data.name);
const fetchedProvider = await externalSecretsStore.getProvider(props.data.name);

providerData.value = {
...(defaultProviderData[props.data.name] || {}),
...provider.data,
...fetchedProvider.data,
};

setConnectionState(provider.state);
setConnectionState(fetchedProvider.state);

if (provider.connected) {
initialConnectionState.value = provider.state;
} else if (Object.keys(provider.data).length) {
if (fetchedProvider.connected) {
initialConnectionState.value = fetchedProvider.state;
} else if (Object.keys(fetchedProvider.data ?? {}).length) {
await testConnection();
}

if (provider.state === 'connected') {
if (fetchedProvider.state === 'connected') {
void externalSecretsStore.reloadProvider(props.data.name);
}
} catch (error) {
Expand All @@ -116,6 +115,10 @@
}

async function save() {
if (!provider.value) {
return;
}

try {
saving.value = true;
await externalSecretsStore.updateProvider(provider.value.name, {
Expand Down Expand Up @@ -143,7 +146,7 @@
const confirmModal = await confirm(
i18n.baseText('settings.externalSecrets.provider.closeWithoutSaving.description', {
interpolate: {
provider: provider.value.displayName,
provider: provider.value?.displayName ?? '',
},
}),
{
Expand All @@ -162,19 +165,23 @@

return true;
}

async function onConnectionStateChange() {
await testConnection();
}
</script>

<template>
<Modal
id="external-secrets-provider-modal"
width="812px"
:title="provider.displayName"
:title="provider?.displayName"
:event-bus="data.eventBus"
:name="EXTERNAL_SECRETS_PROVIDER_MODAL_KEY"
:before-close="onBeforeClose"
>
<template #header>
<div :class="$style.header">
<div v-if="provider" :class="$style.header">
<div :class="$style.providerTitle">
<ExternalSecretsProviderImage :provider="provider" class="mr-xs" />
<span>{{ provider.displayName }}</span>
Expand All @@ -188,7 +195,7 @@
"
:event-bus="eventBus"
:provider="provider"
@change="testConnection"
@change="onConnectionStateChange"
/>
<n8n-button
type="primary"
Expand All @@ -207,7 +214,7 @@
</template>

<template #content>
<div :class="$style.container">
<div v-if="provider" :class="$style.container">
<hr class="mb-l" />
<div v-if="connectionState !== 'initializing'" class="mb-l">
<n8n-callout
Expand Down
43 changes: 26 additions & 17 deletions packages/editor-ui/src/composables/useExternalSecretsProvider.ts
Original file line number Diff line number Diff line change
@@ -1,36 +1,41 @@
import type {
ExternalSecretsProviderWithProperties,
ExternalSecretsProvider,
IUpdateInformation,
ExternalSecretsProviderData,
ExternalSecretsProviderProperty,
ExternalSecretsProviderState,
} from '@/Interface';
import type { Ref } from 'vue';
import type { ComputedRef, Ref } from 'vue';
import { computed, ref } from 'vue';
import { useExternalSecretsStore } from '@/stores/externalSecrets.ee.store';
import { useToast } from '@/composables/useToast';

export function useExternalSecretsProvider(
provider: Ref<ExternalSecretsProvider>,
provider:
| Ref<ExternalSecretsProvider | undefined>
| ComputedRef<ExternalSecretsProvider | undefined>,
providerData: Ref<ExternalSecretsProviderData>,
) {
const toast = useToast();
const externalSecretsStore = useExternalSecretsStore();

const initialConnectionState = ref<ExternalSecretsProviderWithProperties['state'] | undefined>(
'initializing',
);
const initialConnectionState = ref<ExternalSecretsProvider['state'] | undefined>('initializing');
const connectionState = computed(
() => externalSecretsStore.connectionState[provider.value?.name],
() => externalSecretsStore.connectionState[provider.value?.name ?? ''],
);
const setConnectionState = (state: ExternalSecretsProviderWithProperties['state']) => {
externalSecretsStore.setConnectionState(provider.value?.name, state);
const setConnectionState = (state: ExternalSecretsProvider['state']) => {
if (!provider.value) {
return;
}

externalSecretsStore.setConnectionState(provider.value.name, state);
};

const normalizedProviderData = computed(() => {
return Object.entries(providerData.value).reduce(
(acc, [key, value]) => {
const property = provider.value?.properties?.find((property) => property.name === key);
if (shouldDisplayProperty(property)) {
const property = provider.value?.properties?.find((p) => p.name === key);
if (property && shouldDisplayProperty(property)) {
acc[key] = value;
}

Expand All @@ -40,31 +45,35 @@ export function useExternalSecretsProvider(
);
});

function shouldDisplayProperty(
property: ExternalSecretsProviderWithProperties['properties'][0],
): boolean {
function shouldDisplayProperty(property: ExternalSecretsProviderProperty): boolean {
let visible = true;

if (property.displayOptions?.show) {
visible =
visible &&
Object.entries(property.displayOptions.show).every(([key, value]) => {
return value?.includes(providerData.value[key]);
return value?.includes(providerData.value[key] as string);
});
}

if (property.displayOptions?.hide) {
visible =
visible &&
!Object.entries(property.displayOptions.hide).every(([key, value]) => {
return value?.includes(providerData.value[key]);
return value?.includes(providerData.value[key] as string);
});
}

return visible;
}

async function testConnection(options: { showError?: boolean } = { showError: true }) {
async function testConnection(
options: { showError?: boolean } = { showError: true },
): Promise<ExternalSecretsProviderState> {
if (!provider.value) {
return 'initializing';
}

try {
const { testState } = await externalSecretsStore.testProviderConnection(
provider.value.name,
Expand Down
6 changes: 6 additions & 0 deletions packages/editor-ui/src/utils/typeGuards.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,9 @@ export const isResourceMapperValue = (value: unknown): value is string | number
export const isJSPlumbEndpointElement = (element: Node): element is HTMLElement => {
return 'jtk' in element && 'endpoint' in (element.jtk as object);
};

export function isDateObject(date: unknown): date is Date {
return (
!!date && Object.prototype.toString.call(date) === '[object Date]' && !isNaN(date as number)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use instanceof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently date instanceof Date has some limitations as described here. It returns true for invalid date and it fails if dates are passed across boundaries (iframes). The check above is safer.

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting. Might be helpful to add it as a comment because it would confusing to others.

);
}
2 changes: 1 addition & 1 deletion packages/editor-ui/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@
"useUnknownInCatchVariables": false,
"experimentalDecorators": true
},
"include": ["src/**/*.ts", "src/**/*.vue"]
"include": ["src/*.d.ts", "src/**/*.ts", "src/**/*.vue"]
}
Loading