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.

Sign up for GitHub

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

feat: update version notice #4518

Closed
wants to merge 2 commits into from
Closed

feat: update version notice #4518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91aa59a
fix: layout.vue 组件 因 主题sidebar-nav 布局 隐藏 菜单后 转换 为 双列菜单sidebar-mixed-n…
LanceJiang Sep 20, 2024
f655e66
perf: check-updates.vue 检测更新提示窗 通过 worker 进行 请求 通过 message 触发 更新弹窗
LanceJiang Sep 26, 2024
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
13 changes: 13 additions & 0 deletions packages/effects/layouts/src/basic/layout.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,19 @@ watch(
},
);

watch(
() => preferences.app.layout,
async (val) => {
if (val === 'sidebar-mixed-nav' && preferences.sidebar.hidden) {
updatePreferences({
sidebar: {
hidden: false,
},
});
}
},
);

const slots = useSlots();
const headerSlots = computed(() => {
return Object.keys(slots).filter((key) => key.startsWith('header-'));
Expand Down
96 changes: 29 additions & 67 deletions packages/effects/layouts/src/widgets/check-updates/check-updates.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
<script setup lang="ts">
import { h, onMounted, onUnmounted, ref } from 'vue';
import { h, onMounted, onUnmounted, watch } from 'vue';

import { $t } from '@vben/locales';
import { ToastAction, useToast } from '@vben-core/shadcn-ui';

import { createWorker, createWorkFn } from './utils';

interface Props {
// 轮训时间，分钟
checkUpdatesInterval?: number;
Expand All @@ -14,52 +16,35 @@ defineOptions({ name: 'CheckUpdates' });
const props = withDefaults(defineProps<Props>(), {
checkUpdatesInterval: 1,
});

const lastVersionTag = ref('');
let isCheckingUpdates = false;
const timer = ref<ReturnType<typeof setInterval>>();
const rootPath = `${location.origin}/`;
const { toast } = useToast();

async function getVersionTag() {
try {
if (
location.hostname === 'localhost' ||
location.hostname === '127.0.0.1'
) {
return null;
}
const response = await fetch('/', {
cache: 'no-cache',
method: 'HEAD',
});

return (
response.headers.get('etag') || response.headers.get('last-modified')
);
} catch {
console.error('Failed to fetch version tag');
return null;
}
}

async function checkForUpdates() {
const versionTag = await getVersionTag();
if (!versionTag) {
return;
}
const opts = {
checkUpdatesInterval: props.checkUpdatesInterval,
fetchUrl: rootPath,
// immediate: false
};
watch(
() => props.checkUpdatesInterval,
() => {
opts.checkUpdatesInterval = props.checkUpdatesInterval;
},
);
Comment on lines +27 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Changes to checkUpdatesInterval may not affect the worker

The watch function updates opts.checkUpdatesInterval when props.checkUpdatesInterval changes. However, since the worker continues running with the initial options, the updated interval won't take effect unless the worker is notified. Consider sending an update message to the worker or restarting the worker to apply the new interval.

Apply this diff to send updated options to the worker when the interval changes:

 watch(
   () => props.checkUpdatesInterval,
   () => {
     opts.checkUpdatesInterval = props.checkUpdatesInterval;
+    worker.postMessage({ data: { ...opts }, type: 'updateOptions' });
   },
 );

Also, ensure the worker handles the 'updateOptions' message type to update its configuration accordingly.

Committable suggestion was skipped due to low confidence.

const worker = createWorker(createWorkFn, []);

worker.addEventListener('message', (e: any) => {
// {type: 'showNotice', data: 'version'}
if (import.meta.env.MODE === 'production') handleNotice(e.data.data);
});

// 首次运行时不提示更新
if (!lastVersionTag.value) {
lastVersionTag.value = versionTag;
return;
}
const start = (immediate = false) => {
worker.postMessage({ data: { ...opts, immediate }, type: 'start' });
};
const stop = () => {
worker.postMessage({ type: 'stop' });
};
Comment on lines +43 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Terminate the worker upon unmounting to prevent resource leaks

While the stop function sends a 'stop' message to the worker, it does not terminate it. When the component unmounts, the worker remains active, potentially leading to memory leaks. Consider terminating the worker in the onUnmounted hook.

Apply this diff to terminate the worker when the component unmounts:

 onUnmounted(() => {
   stop();
+  worker.terminate();
   document.removeEventListener('visibilitychange', handleVisibilitychange);
 });

Also applies to: 108-112


if (lastVersionTag.value !== versionTag && versionTag) {
clearInterval(timer.value);
handleNotice(versionTag);
}
}
function handleNotice(versionTag: string) {
function handleNotice(/* versionTag: string*/) {
const { dismiss } = toast({
action: h('div', { class: 'inline-flex items-center' }, [
h(
Expand All @@ -79,7 +64,6 @@ function handleNotice(versionTag: string) {
class:
'bg-primary text-primary-foreground hover:bg-primary-hover mx-1',
onClick: () => {
lastVersionTag.value = versionTag;
window.location.reload();
},
},
Expand All @@ -94,36 +78,14 @@ function handleNotice(versionTag: string) {
});
}

function start() {
if (props.checkUpdatesInterval <= 0) {
return;
}

// 每 checkUpdatesInterval(默认值为1) 分钟检查一次
timer.value = setInterval(
checkForUpdates,
props.checkUpdatesInterval * 60 * 1000,
);
}

function handleVisibilitychange() {
if (document.hidden) {
stop();
} else {
if (!isCheckingUpdates) {
isCheckingUpdates = true;
checkForUpdates().finally(() => {
isCheckingUpdates = false;
start();
});
}
start(true);
}
}

function stop() {
clearInterval(timer.value);
}

onMounted(() => {
start();
document.addEventListener('visibilitychange', handleVisibilitychange);
Expand Down
83 changes: 83 additions & 0 deletions packages/effects/layouts/src/widgets/check-updates/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// 创建worker
export const createWorker = (func: () => void, deps?: Array<() => void>) => {
// work 依赖 fns
const depsFncStr = `${deps?.map((dep) => dep.toString()).join(';\n\n') || ''}`;
const blob = new Blob(
[
`
${depsFncStr};
(${func.toString()})();
`,
],
{
type: 'application/javascript',
},
);
return new Worker(window.URL.createObjectURL(blob));
};
LanceJiang marked this conversation as resolved.
Show resolved Hide resolved
export const createWorkFn = () => {
const opts = {
checkUpdatesInterval: 1, // min
fetchUrl: '/',
immediate: true,
};
let timer: any = null;
const stop = () => {
clearInterval(timer);
};
const temp: Worker = globalThis as any;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Type Mismatch: globalThis Should Be WorkerGlobalScope

At line 28, temp is declared as Worker, but globalThis in a worker context refers to WorkerGlobalScope, not Worker. This mismatch can lead to type inconsistencies and confusion when using worker-specific properties and methods.

Update the type of temp to WorkerGlobalScope to accurately represent the worker's global context:

-  const temp: Worker = globalThis as any;
+  const temp: WorkerGlobalScope = globalThis as any;

Or, for more specific typing, use DedicatedWorkerGlobalScope:

-  const temp: Worker = globalThis as any;
+  const temp: DedicatedWorkerGlobalScope = globalThis as any;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const temp: Worker = globalThis as any;
const temp: WorkerGlobalScope = globalThis as any;
```
Alternatively, using the more specific type:
```suggestion
const temp: DedicatedWorkerGlobalScope = globalThis as any;

let lastVersionTag = '';
const getVersionTag = async () => {
try {
const response = await fetch(opts.fetchUrl, {
cache: 'no-cache',
method: 'HEAD',
});

return (
response.headers.get('etag') || response.headers.get('last-modified')
);
} catch {
console.error('Failed to fetch version tag');
return null;
}
};
const doFetch = async () => {
const versionTag = await getVersionTag();
console.error(versionTag, 'versionTag');
if (!versionTag) return;
// 首次运行时不提示更新
if (!lastVersionTag) {
lastVersionTag = versionTag;
return;
}
if (lastVersionTag !== versionTag) {
stop();
temp.postMessage({ data: versionTag, type: 'showNotice' });
}
};
temp.addEventListener('message', async (event: any) => {
// { type: 'start' | 'stop' }
switch (event.data.type) {
case 'start': {
{
const data = event.data.data;
if (data.checkUpdatesInterval)
opts.checkUpdatesInterval = data.checkUpdatesInterval;
if (data.fetchUrl) opts.fetchUrl = data.fetchUrl;
if (data.immediate) {
Comment on lines +64 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate Incoming Data in Message Handler

The code updates opts based on event.data.data without validation:

const data = event.data.data;
if (data.checkUpdatesInterval)
  opts.checkUpdatesInterval = data.checkUpdatesInterval;
if (data.fetchUrl) opts.fetchUrl = data.fetchUrl;
if (data.immediate) {
  await doFetch();
}

To prevent potential errors or security issues, validate the incoming data:

if (typeof data.checkUpdatesInterval === 'number' && data.checkUpdatesInterval > 0)
  opts.checkUpdatesInterval = data.checkUpdatesInterval;
if (typeof data.fetchUrl === 'string') opts.fetchUrl = data.fetchUrl;
if (typeof data.immediate === 'boolean' && data.immediate) {
  await doFetch();
}

await doFetch();
}
if (timer) stop();
timer = setInterval(doFetch, opts.checkUpdatesInterval * 60 * 1000);
}
break;
}
case 'stop': {
stop();
break;
}
}
});
return temp;
};
Loading