Skip to content

add image-compression#71

Merged
nighca merged 6 commits intoqiniu:masterfrom
doxiaodong:image-compression
Feb 7, 2018
Merged

add image-compression#71
nighca merged 6 commits intoqiniu:masterfrom
doxiaodong:image-compression

Conversation

@doxiaodong
Copy link
Copy Markdown
Contributor

@doxiaodong doxiaodong commented Jan 31, 2018

Comment thread README.md Outdated
// ...
```

- imageCompression
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

感觉跟上边的命名风格保持一致的话应该是 compressImage

const compressionIfNecessary = rule => {
const loaders = rule.use
const firstLoaderName = loaders[0].__loaderName__
if (firstLoaderName !== 'file-loader' || !utils.isImage(rule.test)) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

判断是不是 image 的话,是不是用 add-transform 那的 extension 更准确一点?而且下边我看会去改 loaders,要不干脆把这事儿挪到 add-transform 里做(那边判断下 envoptimization)?

虽然这样 add-transform 里边逻辑会趋于越来越多,不过我们可以回头再去想怎么拆那边,比如按 extension 拆开逻辑组织

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

这个和 extract-style 是统一类型的啊,我和那个保持统一了

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

嗯,但是这边 isImage 的判断方式我有点慌啊

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

我没想到例外的

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

这个是建立在现在的 test 全是通过 makeExtensionPattern 创建的,我能想到的情况是,比如我们 transform 填后缀名那边支持 * 了,然后估计就会对应创建一个类似 .*$test,那它也就被识别成 image 了。。

我觉得主要点在于,这个 test 是只考虑到给 webpack 用的,不会预期我们自己会去依据它的值做一些判断逻辑;这也是 extract-style 那边会特地通过一个 __loaderName__ 来获取可靠的 loader 名的原因;所以这边如果像现在这样组织的话,那也把 extension 信息通过某个我们自己的字段给出来,会更可靠一点。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

在 rule 里新加一个 extension 字段?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

__extension__?这个我倒是可以接受

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

那个 issuer 字段是 webpack 本身就有的么

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

对的

@doxiaodong doxiaodong force-pushed the image-compression branch 2 times, most recently from 6f1229e to bf6a697 Compare February 6, 2018 01:52
@doxiaodong doxiaodong force-pushed the image-compression branch 2 times, most recently from 81efd88 to 2cfe002 Compare February 7, 2018 01:50
@doxiaodong
Copy link
Copy Markdown
Contributor Author

go

Copy link
Copy Markdown
Collaborator

@nighca nighca left a comment

Choose a reason for hiding this comment

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

🎸

@nighca nighca merged commit f9e5542 into qiniu:master Feb 7, 2018
@nighca nighca mentioned this pull request Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants