Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: dependency check before starting a task #2421

Merged
merged 5 commits into from
Jul 17, 2019

Conversation

chenbin92
Copy link
Collaborator

@chenbin92 chenbin92 commented Jul 11, 2019

  • 执行任务前检查依赖是否安装,如果未安装则给出对应的提示
  • 日志流写入到 Global
  • 日志流写入到当前 Xterm 中

image

@chenbin92 chenbin92 self-assigned this Jul 11, 2019
@chenbin92 chenbin92 changed the title feat: dependency check before starting a task [WIP]feat: dependency check before starting a task Jul 11, 2019
@chenbin92 chenbin92 changed the title [WIP]feat: dependency check before starting a task feat: dependency check before starting a task Jul 12, 2019
@chenbin92 chenbin92 requested a review from alvinhui July 12, 2019 01:29
@@ -68,11 +68,17 @@ function useDependency(diableUseSocket) {
await dependenciesStore.reset();

setResetModal(false);
globalTerminalStore.show();

if (showGlobalTerminal) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

如果要封装 showGlobalTerminal 这个参数的话,建议是把 globalTerminalStore.show(); 封装一下,这样控制掉所有的这个场景,不仅仅是 reset 这个场景。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

怎么封装,没理解

}

if (!diableUseSocket) {
useSocket('adapter.dependency.reset.data', writeGlobalLog);
if (writeChunk) {
useSocket('adapter.dependency.reset.data', data => writeChunk(data));
Copy link
Collaborator

Choose a reason for hiding this comment

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

建议在外部处理这个逻辑,不封装到 useDependency 内。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 外部处理是指在哪里处理?
  2. 不封装到 useDependency 的理由?

封装到 useDependency 的理由:封装 useDependency 本来是为了共用的,与写入到 Global 保持一致都在 useDependency 处理。反而写入到 Global 可以写在 useDependency 而 WriteChunk 不可以写的考虑是怎样的呢

Copy link
Collaborator

Choose a reason for hiding this comment

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

如果是这个考虑的话,

那是不是所有有 writeGlobalLog 的地方都同时 writeChunk ?

function writeLog(data) {
    writeGlobalLog(data);
    writeChunk(data);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 场景:在这里安装依赖的前提是启动调试服务,日志流都在工程面板输出合理
  2. 那是不是所有有 writeGlobalLog 的地方都同时 writeChunk ?
    肯定不是,log 是人为输出的可读的操作日志,chunk 是类似 npm install 和 npm run start 的流数据


async function onStart() {
try {
writeLog(type);
await taskStore.start(type);
if (!taskStore.dataSource.installed) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

建议是将这个状态放入 dependenciesStore。

这里应该是:

if (!dependenciesStore.dataSource.installed) {
     setInstallDependencyVisible(true);
     return;
}

installDependencyVisible,
onInstallDependencyCancel,
onInstallDependencyOk,
} = useTask({ type, writeLog, writeChunk });
Copy link
Collaborator

@alvinhui alvinhui Jul 15, 2019

Choose a reason for hiding this comment

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

如果将 installed 状态放入 dependenciesStore ,则 useTask 内部不需要包含 useDependency 逻辑?
可以直接使用 useDependency 内的状态:

  • installDependencyVisible => onResetModal
  • onInstallDependencyCancel => onResetModalCancel
  • onInstallDependencyOk => onResetModalOk

Copy link
Collaborator Author

@chenbin92 chenbin92 Jul 17, 2019

Choose a reason for hiding this comment

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

是不是封装到 dependency/index.ts 中更合适呢

同下面一个问题:#2421 (diff)

  1. 放在 useDependency 的检查时机是什么呢,每次都去请求一遍?如果是这样的话我觉得放在 task 更合理,触发时机就是 start, build, lint 等时机,这个字段也是工程需要的,dependency 并太在意是否安装过

image

  1. onResetModal 这个语义太难理解了

@alvinhui
Copy link
Collaborator

请同时处理 QuickDevPanel

@@ -20,6 +20,8 @@ export default class Task implements ITaskModule {

private status: object = {};

private installed: boolean = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

是不是封装到 dependency/index.ts 中更合适呢

@chenbin92 chenbin92 changed the base branch from iceworks/release-3.0.0-beta.4 to iceworks/release-3.0.0-beta.5 July 15, 2019 13:43
@chenbin92 chenbin92 changed the base branch from iceworks/release-3.0.0-beta.5 to iceworks/release-3.0.0-beta.6 July 17, 2019 08:36
@chenbin92
Copy link
Collaborator Author

请同时处理 QuickDevPanel

QuickDevPanel 和 quickBuildPanel 已加

@chenbin92 chenbin92 mentioned this pull request Jul 17, 2019
6 tasks
@alvinhui
Copy link
Collaborator

installed 在 taskStore 感觉真心不合适呀。
「依赖是否已安装」,不是应该在 dependenciesStore 里吗?什么时候初始化呢,refresh 的时候呀。

@alvinhui alvinhui closed this Jul 17, 2019
@alvinhui alvinhui reopened this Jul 17, 2019
@chenbin92
Copy link
Collaborator Author

chenbin92 commented Jul 17, 2019

installed 在 taskStore 感觉真心不合适呀。
「依赖是否已安装」,不是应该在 dependenciesStore 里吗?什么时候初始化呢,refresh 的时候呀。

不只是初始化,还需要考虑用户在直接在目录里面删除,然后每次获取的时候在去执行一遍?

@alvinhui
Copy link
Collaborator

installed 在 taskStore 感觉真心不合适呀。
「依赖是否已安装」,不是应该在 dependenciesStore 里吗?什么时候初始化呢,refresh 的时候呀。

不只是初始化,还需要考虑用户在直接在目录里面删除

refresh 是每次都会做的呀。

@chenbin92
Copy link
Collaborator Author

installed 在 taskStore 感觉真心不合适呀。
「依赖是否已安装」,不是应该在 dependenciesStore 里吗?什么时候初始化呢,refresh 的时候呀。

不只是初始化,还需要考虑用户在直接在目录里面删除

refresh 是每次都会做的呀。

我的意思是:用户在编辑器中删除了 node_modules,此时用户启动调试服务,按照现在的做法,在启动之前就会检查一次,如果放在 Deps 中,则每次启动之前都要另外发一个请求去 refresh

image

@alvinhui alvinhui merged commit 0f84a7e into iceworks/release-3.0.0-beta.6 Jul 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the iceworks/check-dependency branch July 17, 2019 12:34
@chenbin92
Copy link
Collaborator Author

从另外一个角度考虑:启动工程对应的是 package.json 中的 scripts 脚本,假设我们在命令行中执行 npm run start 发现依赖没有安装,这个时候会做什么呢,所有社区也有另外一种做法,将命令设计如下:

"scripts": {
   "start": "npm install &&  react-scripts dev"
}

这里想表达的是这里是存在对应关系的,可以放在一起处理,至于是否更合理,我觉得需要整体看下

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants