-
Notifications
You must be signed in to change notification settings - Fork 836
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
feat: upgrading esm targets to targeting es2017 #2472
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2472 +/- ##
==========================================
- Coverage 92.62% 92.60% -0.02%
==========================================
Files 137 137
Lines 5017 5017
Branches 1060 1060
==========================================
- Hits 4647 4646 -1
- Misses 370 371 +1
|
I think this should still remain es5, you don't have es2017 in browser |
I agree with @obecny especially since the ESM build exists mostly to serve the browser usecase. |
I doubt there are any browsers which support native ESM but don't support await, but I wasn't really thinking of native ESM support as much as build tooling. After thinking a little more though I think this is probably fine. The non-esm builds already target 2017 so I'm not sure why esm was pointing to es5 in the first place. ESM builds were contributed by @t2t2 in #2112 so perhaps they can shed some light on that? |
hmm you right let's wait for @t2t2 would like to know if there were reasons for that or it was just mistake. |
@t2t2 isn't a regular contributor so i'm not sure how quickly he will reply here. We can wait for some time, but if it doesn't come soon i'd rather not wait too long. |
we can wait 24 hours and then just move forward |
Yeah, while browsers that support esm modules also support modern syntax like async/await, the issue is you generally don't see much usage of esm modules directly via browser as no matter how much http2 coolaid you've consumed, it's performance has still been awful for real apps (webpack blog post in 2016, vue/vite author in 2021). At most the use case for So this leaves esm files to be mainly used by bundlers for tree shaking. Good news here is bundlers don't care about how modern syntax is. Bad news is bundlers don't care about how modern syntax is - it's the job of transpilers like babel. So for users who still want to support older browsers (IE) they'd need dependencies to work in them or transpile it themselves. So let's check webpack's babel-loader documentation
How about rollup's babel plugin:
So that's 2 most popular ways saying don't transpile your dependencies (= dependencies should ship older code). This has way of thinking challenged in default templates for frameworks, for example create-react-app switched to transpiling dependencies by default in v2, vue-cli offers a config option for dependencies to transpile, angular is the only one explicilty saying target es2015; and also from some library authors (preact's microbundlers modern mode, svelte author's opinion) Question here becomes can you really afford to go modern considering who's gonna use opentelemetry. Like if I were to create/maintain a library that I'd expect users to be more up to date on, yeah I'd provide a modern build so those who don't use IE can enjoy smaller size (or if IE support was to be expected write code in a way that avoids newer features). But in case of opentelemetry you can expect a lot more users who want to throw it to some old angularjs app they have with a webpack config that is held together with copy-pasted snippets from ages ago, and that still want IE support. So for ESM pr the less breaking way was to just match cjs targets so bundlers that grabbed esm files without transpiling still worked. So basically there's 2 options:
|
Consider me another vote for "just leave it at esm5". |
Thank you @t2t2 for the explanation. @legendecas I assume since you 👍 his suggestion to leave as es5 you are ok with closing this PR? |
Which problem is this PR solving?
Short description of the changes
await
.await
import
statement