-
Notifications
You must be signed in to change notification settings - Fork 33
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 some todos #289
Merged
Merged
Fix some todos #289
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
5871b91
hooks for file url
nighca 12addf9
disable eslint rule no-redeclare
nighca ee4a7db
timeout for api client
nighca dfbf8dc
remove useless TODOs
nighca a7babcc
remove userInfo assertion in validateName
nighca b5b7528
keep consistent order for sprites & sounds
nighca ad3fc85
unit test for filename & sripExt
nighca 89bd40e
typo
nighca 5ed6f86
use file type to construct blob
nighca File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
|
||
import { getMimeFromExt } from '@/utils/file' | ||
import { extname } from '@/utils/path' | ||
import type { Disposer } from './disposable' | ||
import { Cancelled } from '@/utils/exception' | ||
|
||
export type Options = { | ||
/** MIME type of file */ | ||
|
@@ -40,10 +42,16 @@ export class File { | |
})) | ||
} | ||
|
||
// TODO: remember to do URL.revokeObjectURL | ||
async url() { | ||
async url(onCleanup: (disposer: Disposer) => void) { | ||
let cancelled = false | ||
onCleanup(() => { | ||
cancelled = true | ||
}) | ||
const ab = await this.arrayBuffer() | ||
return URL.createObjectURL(new Blob([ab])) | ||
if (cancelled) throw new Cancelled() | ||
const url = URL.createObjectURL(new Blob([ab], { type: this.type })) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这里的this.type我记得不会保存(保存到cloud之后再加载下来type信息会丢失) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 嗯现在调用上传接口的时候没有把 mime type 提交,这个我单独 PR 解决下吧,得去看下对象存储的接口 |
||
onCleanup(() => URL.revokeObjectURL(url)) | ||
return url | ||
} | ||
} | ||
|
||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
考虑不要暴露url(),让用户使用URL.createObjectURL自己创建和销毁,可以用
useFileUrl(file): url
包装watchEffect来监听文件改动?另外,Blob中的type是必须的,之前遇到过音频无法播放/下载扩展名错误的问题。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
手动包装一个Disposer有些多余
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个是考虑将来
url()
有可能不返回 object url,而是返回 file 本身的 public url(file loaded from public url 可以视作一种特殊的 lazy file,它知道文件内容对应的 public url),所以把获得 URL 的逻辑做在 File 上这个没太理解,这里是希望
url()
返回 type 信息?“包装一个 Disposer”是指?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
噢我明白你意思了,是指
URL.createObjectURL(new Blob([ab]))
构造 blob 的时候要传入 type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果要有这个逻辑应该分开?因为 public url不需要 revoke 而从blob创建的url需要。
对的
是指创建 URL 时通过传入 onCleanup 来清除 URL,如果按我说的由外部来创建/清除就不需要这样传入onCleanup的逻辑。我觉得传入onCleanup这个行为有些复杂/不别要
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
将来借助 https://github.com/tc39/proposal-explicit-resource-management 这个也是一个可能的思路,它对使用者来说是简洁的;不过那个东西我没怎么用过还,跟 vue/react 结合使用会不会有坑还不清楚
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
看了下getSize这个方法,应该还是可以在resolve之后手动调用revoke清理url,和这个比较像:
如果需要使用方显式地声明生命周期,这个接口从复杂度来说可能和和不封装也一样。我个人更偏向“谁创建谁销毁”这种接口形式。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
当然可以由 getSize 去做,但是就像上面提到的,
“确保没有内存泄漏”会比接口简单更重要
我做在
File url()
中不是希望屏蔽 create & revoke Objecet URL 的复杂度,而是希望屏蔽 object url 跟 public url 的差异File url()
去 create Object URL,也由File url()
去 revoke,这不也符合“谁创建谁销毁”吗?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
销毁的时机是又用户来决定而不是 File.url() 来决定的
public url 如果也需要传入一个 cleanup 参数我认为是不太合理的
这个应该就是 CR 需要解决的问题了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
对,用户调用 File.url() 并使用其结果,所以用户决定 File.url() cleanup 的时机,这不还是“谁创建谁销毁”吗?
不是 public url 需要传入 clean up,而是一份可能是 public url 的数据需要 clean up
这样的问题依赖 CR 去发现才是不得已的,能让工具帮助检查当然更好