fix(plugin-legacy)!: support browserslist and update default target #11318
Conversation
sapphi-red
added
plugin: legacy
p4-important
packages/plugin-legacy/src/index.ts
Outdated
| const targets = options.targets || 'defaults' | ||
| const targets = options.targets |
There was a problem hiding this comment.
Maybe it's better to avoid this breaking change if setting targets: {} on the user-side works.
There was a problem hiding this comment.
I think options.targets's value should be undefined so that @babel/preset-env to detect browserslist, it's not breaking change. The default value in document is reference to @babel/preset-env's targets, maybe I should modify document about targets.
Sorry, I am not goot at documentation.
There was a problem hiding this comment.
In the last meeting, we discussed this one. We think browserslist should have precedence over the default value. Also the current default value is not ideal, given that 'defaults' is not so different from Vite's (non-legacy) default browser target and plugin-legacy users would use a lower target than 'defaults' usually.
I think we could include the default value change in this PR. But if you are not interested in investigating about that, we can go with this PR as-is. I'll create follow up PR in that case.
There was a problem hiding this comment.
I wonder that what value will be set. In my opinions, the value undefined is most appropriate default value, because any value than undefined will stop detecting browserslist.
I have updated documentation about targets, if you want to modify default targets, take control of it.
There was a problem hiding this comment.
because any value than
undefinedwill stop detectingbrowserslist.
Yes, so we need to call browserslist on our side first before calling babel.
There was a problem hiding this comment.
Sorry for taking time. I've been thinking about the new default target.
defaults is a similar target to our target for non-legacy (es2020,edge88,firefox78,chrome87,safari14) but I noticed that defaults includes "Opera Mini" (uses Opera 12.1 internally and doesn't completely support ES5) and the result will be very different.
So maybe we could go with the current default (defaults).
Or we could widen the target a bit more to > 0.1%, last 2 versions, Firefox ESR (this covers 93.9 %). defaults is an alias of > 0.5%, last 2 versions, Firefox ESR, not dead and covers 88.9%.
@patak-dev @ArnaudBarre What do you think?
FYI swc tries browserslist and fallbacks to {} (transpile everything) which is same with babel.
There was a problem hiding this comment.
No opinion at all on this, I don't work on any application that need legacy support. But from my experience, if ie11 is included in the default that good enough, people who want more precise support will probably provide a list
There was a problem hiding this comment.
I had finished a projects runs on android 7 (chrome 61) before a few months, so I don't think chrome87 is not best choice in my opinions, and I have searched a link about browsers version stats, maybe it is helpful for you.
There was a problem hiding this comment.
IIUC the context, I think a wider defaults for legacy makes sense here. Why would we use the same non-legacy default for legacy? That would result in the same browser support when adding the plugin without config, no? Or maybe I'm misunderstanding the issue. I trust your judgment here @sapphi-red.
There was a problem hiding this comment.
last 2 versions and not dead, > 0.3%, Firefox ESR seems to be a good one to choose. This includes IE 11 and iOS 12.x that is still maintained a bit.
Let's go with this one.
KAROTT7
force-pushed
the
improve-plugin-legacy-support-browserslist
branch
from
55f605c
to
84f5f68
Compare
sapphi-red
changed the title
sapphi-red
changed the title
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
|
@sapphi-red Thank you for suggests! |
|
Let's merge this one and release a new major for plugin-legacy so we can announce it together with Vite 4.1. Thanks for pushing this one to the finish line @KAROTT7! |
…itejs#11318) Co-authored-by: 翠 / green <green@sapphi.red> fixes vitejs#2476
Description
fixes #2476
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).