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

optimization.addPolyfill 支持指定 polyfill 方式 #126

Merged
merged 15 commits into from
Jul 24, 2020

Conversation

surmon-china
Copy link
Contributor

@surmon-china surmon-china commented Jul 10, 2020

optimization.addPolyfill 支持传入字符串值指定 polyfill 方式:

  • 默认值:global,即:使用基于 @babel/polyfill 的 polyfill,polyfill 会被作为独立的包被页面前置引用,会污染全局内置对象,适合宿主应用;
  • 可选值:runtime,使用 @babel/plugin-transform-runtime 实现的 polyfill,生成辅助函数以替换特定方法,不会污染全局命名空间,适用于工具类库(但目前使用 corejs2,所以还不支持实例方法,如:[].includes

更新为 corejs@3 了,但没有测试

This was referenced Jul 10, 2020
@surmon-china
Copy link
Contributor Author

@huangbinjie @Luncher @nighca @lzfee0227

代码先改完了,@huangbinjie 有空的话测试下啊

Copy link

@lzfee0227 lzfee0227 left a comment

Choose a reason for hiding this comment

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

看不懂
@nighca @Luncher

package.json Outdated
@@ -32,7 +32,7 @@
"image-webpack-loader": "^4.6.0",
"immutability-helper": "^2.5.0",
"jest-cli": "^24.7.1",
"less": "^3.9.0",
"less": "3.9.0",

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

可能是我错误的测试得出了错误的结论。

现在看来,是 less-loader v6.x API 变了,options 变成了 lessOptions
再看来,less 可以升级为最新,应该没问题

* @author Surmon <[email protected]>
*/

exports.PolyfillType = {

Choose a reason for hiding this comment

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

首字母应该大写吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我把这东西定义为常量,所以就大驼峰了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

这边没有“常量大驼峰”的习惯..毕竟不是 TS enum

build-config.md Outdated
是否开启自动 polyfill 功能,开启后会根据 `targets.browsers` 参数自动打包对应的 polyfill,并在作为独立的包被页面前置引用,`true` 启用,`false` 禁用
是否开启自动 polyfill 功能,开启后会根据 `targets.browsers` 参数自动实现对应的 polyfill;`true` 启用,`false` 禁用

- **`optimization.polyfillType`**

Choose a reason for hiding this comment

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

这个是否要/能跟 addPolyfill merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

看各位大佬的意思

Copy link
Collaborator

Choose a reason for hiding this comment

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

建议合并,builder 的配置目前习惯对较复杂的配置项,保持相同项、支持不同类型的值;如 transforms.xxxoptimizations.transformDeps

"semver": "^5.5.0",
"style-loader": "^0.19.0",
"stylus": "^0.54.5",
"stylus": "^0.54.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

NIP:这玩意是不是也可以去掉= =,和 sass 去掉的目的一样

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这要问 @nighca

Copy link
Collaborator

Choose a reason for hiding this comment

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

@huangbinjie 这个东西不像 sass 那么坑,可以先不动它..

build-config.md Outdated Show resolved Hide resolved
@@ -45,6 +45,7 @@
},
"optimization": {
"addPolyfill": true,
"polyfillType": "default",
Copy link
Contributor

Choose a reason for hiding this comment

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

这个字段如果跟 addPolyfill 合并起来会不会好些?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

看各位大佬的意思

Copy link
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.

除了配置的 API,我这没问题;插件的配置与使用相关 @Luncher @huangbinjie 更熟悉,麻烦把个关~

@surmon-china
Copy link
Contributor Author

surmon-china commented Jul 21, 2020

除了配置的 API,我这没问题;插件的配置与使用相关 @Luncher @huangbinjie 更熟悉,麻烦把个关~

建议合并为:"polyfill": "<string>",注意,是 替换 "addPolyfill""polyfill",类型是 boolean 替换为 string

可选值为:

value desc
"none" 不开启 polyfill
"default" 开启基于 @babel/polyfillpolyfill
"runtime" 开启基于 @babel/plugin-transform-runtimepolyfill

各位大佬看怎么样 @nighca @lzfee0227 @Luncher @huangbinjie

build-config.md Outdated

使用什么形式的 polyfill

- 默认值:`default`,即:使用基于 `useBuiltIns` 的 polyfill,polyfill 会被作为独立的包被页面前置引用,会污染全局内置对象,适合大型应用;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
- 默认值:`default`,即:使用基于 `useBuiltIns` 的 polyfill,polyfill 会被作为独立的包被页面前置引用,会污染全局内置对象,适合大型应用
- 默认值:`default`,即:使用基于 `useBuiltIns` 的 polyfill,polyfill 会被作为独立的包被页面前置引用,会污染全局内置对象,适合宿主应用

Copy link
Collaborator

Choose a reason for hiding this comment

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

适合普通项目宿主的话,感觉一般是表示有人寄生,被寄生一方才叫宿主

@nighca
Copy link
Collaborator

nighca commented Jul 22, 2020

替换 "addPolyfill""polyfill",类型是 boolean 替换为 string

@surmon-china 替换字段名是为了保持老字段的兼容吗?

是的话为啥不考虑合并字段,让这个字段支持 booleanstring如前所述,这个是有先例的做法)?不是的话,为这个做不兼容变更不值得

我的粗略想法是:

optimization.addPolyfill

value desc
false 不开启 polyfill
true 开启 polyfill,使用默认 type(同 default
"default" 开启基于 @babel/polyfillpolyfill
"runtime" 开启基于 @babel/plugin-transform-runtimepolyfill

(如果 default 能换个名字就更好了...)

@@ -39,7 +40,10 @@ module.exports = (webpackConfig, srcDir, srcPath, { addPolyfill }) => {
[
path.join('./', srcDir, '/') + mockFile
]: (
`require('@babel/polyfill')`
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个的来源是?我看按这边的意思是

import "core-js/stable";
import "regenerator-runtime/runtime";

require("core-js");
require("regenerator-runtime/runtime");

这边 import / require & core-js / core-js/stable 有说法吗

Copy link
Collaborator

@nighca nighca Jul 22, 2020

Choose a reason for hiding this comment

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

本地验证一下行为吧

Copy link
Contributor Author

@surmon-china surmon-china Jul 22, 2020

Choose a reason for hiding this comment

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

参考信息:

  1. 原有的 @babel/polyfill 源码没有啥参考价值,因为和它文档说的也对不上
  2. babel 是靠这几段代码来确定 如何替换指定的 polyfill 语句的(按顺序来)

所以,可以看出,require('core-js') 是比 require('core-js/stable') 替换的东西要多的,前者大概 335 条规则,后者 216 条,diff 结果在这里

Copy link
Contributor Author

Choose a reason for hiding this comment

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

所以区别基本上就是:

core-js: 使用 core-js 全部功能打补丁。

core-js/stable: 仅仅使用稳定的 core-js 功能 - ES 和 web 标准。(This folder contains entry points for all stable core-js features with dependencies. It's the recommended way for usage only required features. )

Copy link
Collaborator

@nighca nighca Jul 23, 2020

Choose a reason for hiding this comment

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

OK 👍 本地验证也符合预期就行

Copy link
Contributor Author

Choose a reason for hiding this comment

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

本地还没验证... 等我再验证下,先不要管这个 PR

Copy link

@lzfee0227 lzfee0227 Jul 23, 2020

Choose a reason for hiding this comment

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

build-config.md Outdated
| value | desc |
| ------------- | ------------- |
| `false` | 不开启 `polyfill` |
| `true` | 开启 `polyfill`,使用 `global` 方式的 `polyfill` |

This comment was marked as resolved.

Copy link
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.

🎎

Copy link
Contributor

@huangbinjie huangbinjie left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 13 to 14
}
exports.isRuntimePolyfill = type => {

Choose a reason for hiding this comment

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

\n

@@ -8398,8 +9722,8 @@
},
"less": {
"version": "3.9.0",
"resolved": "https://registry.npmjs.org/less/-/less-3.9.0.tgz",
"integrity": "sha512-31CmtPEZraNUtuUREYjSqRkeETFdyEHSEPAGq4erDlUXtda7pzNmctdljdIagSb589d/qXGWiiP31R5JVf+v0w==",
"resolved": "https://r.cnpmjs.org/less/download/less-3.9.0.tgz",

Choose a reason for hiding this comment

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

为什么就这几个货换 cnpm 了
一点都不纯粹

@huangbinjie
Copy link
Contributor

optimization.addPolyfill

@nighca @surmon-china 你看能不能这么改,支持 object,以支持更多的配置

value desc
false 不开启 polyfill
true 开启 polyfill,使用默认 type(同 default
object { polyfillTzype?: 'default', 'runtime', proposal?: boolean }

@nighca
Copy link
Collaborator

nighca commented Jul 24, 2020

optimization.addPolyfill

你看能不能这么改,支持 object,以支持更多的配置

@huangbinjie 我不倾向现在改,主要是考虑:

  1. 目前没看到对“是否支持 proposals”定制的需求(builder 原则上是能不给定制的就不暴露定制能力,否则就等于再造了个 webpack,没啥意义)
  2. 真的需要对 proposals 支持程度进行定制的话,简单的 true / false 又大概率不能满足需求(至少要能指定支持到 stage-n吧?),那样就比较复杂了
  3. 现在这么做了(boolean / string),不妨碍以后在真的需要的时候再去支持 object 格式的配置,代价是不大的

@surmon-china
Copy link
Contributor Author

已在本地测试,true false "global" "runtime" 均符合预期

Copy link
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 99ec547 into qiniu:master Jul 24, 2020
@nighca nighca changed the title v1.18.0 (polyfill type) optimization.addPolyfill 支持指定 polyfill 方式 Jul 24, 2020
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.

5 participants