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

chore: add esm2015 entry for web apps aiming at modern browsers #2556

Merged

Conversation

echoontheway
Copy link
Contributor

@echoontheway echoontheway commented Oct 21, 2021

Which problem is this PR solving?

  • add esm2015 entry point for web apps aiming at modern browsers
  • reduce bundle size of web apps aiming at modern browsers

Fixes

Short description of the changes

  • add a tsconfig.esm2015.json for each package to compile TS using es6 synax
  • add a esm2015 entry point for each package.json

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Enhancements above is tested by a simplified web app.

  • the tool to pack the web app is vite, the tool to analyze bundle size is rollup-plugin-visualizer

  • use esm2015 entry point

    • pack config
    cfbc2c4e-ddcd-4b7c-a38d-54e9a6e3b9cd
    • pack command
      npm run build
    • bundle size
      133KB, details
  • use esm entry point

    • pack config
    117f28b5-e739-42f1-97c5-f5324fdfc067
    • pack command
      npm run build
    • bundle size
      186KB, details

@echoontheway echoontheway requested a review from a team October 21, 2021 09:15
@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #2556 (36705eb) into main (dded3f8) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2556      +/-   ##
==========================================
- Coverage   93.36%   93.31%   -0.06%     
==========================================
  Files         131      131              
  Lines        4932     4923       -9     
  Branches     1078     1078              
==========================================
- Hits         4605     4594      -11     
- Misses        327      329       +2     
Impacted Files Coverage Δ
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts 68.96% <0.00%> (-11.60%) ⬇️
...kages/opentelemetry-api-metrics/src/api/metrics.ts 85.71% <0.00%> (-5.20%) ⬇️
...opentelemetry-api-metrics/src/NoopMeterProvider.ts 100.00% <0.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

@legendecas
Copy link
Member

I'm curious about the entry point "esm2015". Is the entry point name "esm2015" widely adopted with any tools?

For shipping a new build variant, I'm wondering that what if people would like to perform tree-shaking but don't need to transpile to lower ECMAScript versions? Should we ship more entry points and build variants like "esm2016", or "esm2017"? Would it be better if we ship a new ESM build variant that is merely striped typescript annotations and leaves all other syntaxes as is?

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

This looks nice and promising, definitely nice improvement. Perhaps @MSNev can be interested in this too ?

@CAaRrLl
Copy link

CAaRrLl commented Oct 25, 2021

I could not find the convention of the field "esm2015" of package.json. For me who would like to enable esm2015 entry point needs to know this new rule and set "esm2015" into the mainFields. ESM2015 which is widely supported by many modern browsers and covers most syntax. Would it be better if just change the entry point "module" to use esm2015?

@legendecas
Copy link
Member

@CAaRrLl: I could not find the convention of the field "esm2015" of package.json. For me who would like to enable esm2015 entry point needs to know this new rule and set "esm2015" into the mainFields. ESM2015 which is widely supported by many modern browsers and covers most syntax. Would it be better if just change the entry point "module" to use esm2015?

That's been discussed at #2472 (comment) recently and we concluded that we are not going to ship modern javascript on the "module" entrypoint in the near future.

@dyladan
Copy link
Member

dyladan commented Oct 26, 2021

I'll leave this to @obecny and @MSNev who have more browser experience (and this one affects bundle size so Nev will be happy) but generally these changes look ok to me and I have no real objections. My only concern is that shipping more and more builds in the same package bloats the package size, but I don't think it's a real problem quite yet.

@obecny
Copy link
Member

obecny commented Oct 26, 2021

I'll leave this to @obecny and @MSNev who have more browser experience (and this one affects bundle size so Nev will be happy) but generally these changes look ok to me and I have no real objections. My only concern is that shipping more and more builds in the same package bloats the package size, but I don't think it's a real problem quite yet.

I wouldn't mind to have extra entry point. 1st - it doesn't harm or is causing any regression. 2nd - it gives some benefit for those eager to use it. So in general "yes" from me

@MSNev
Copy link
Contributor

MSNev commented Oct 26, 2021

I'm actually not too fussed on this (I can have it either way).

Avoiding polyfill's is always a good thing for size savings and this change (I think) is really only helps users that are consuming the pre-built *.js. And how many people are really only targeting esm2015 for not just this component but for their entire app??

If we deployed to a CDN then I say absolutely (for those users that can target only modern browser) but as we dont I'm not against it either as there will be some percentage (unknown) that would find this useful.

But I think generally, anyone using would probably pick and choose the components they want and their packager will take care of tree shaking...

@t2t2
Copy link
Contributor

t2t2 commented Oct 27, 2021

Separate resolve field definitely a good way to bypass the issue of module field & babel ignore node_modules inertia, tho it not being a standard field (hence having to configure your bundler to use esm2015 as entry field as shown in screenshots) does mean that the amount of users who end up using this is quite low (since you'd have to discover this option being available - either discovering it in package.json and wondering what it is or finding this PR). Maintenance burden is probably low enough that cost:benefit(ing users) ratio doesn't really matter, but there's no documentation updates in this PR that explain a) this option is available b) how to use it


Worth considering: Why stop at just target: es6? If there is an option for modern-er code that the user has to explicitly opt into, then there's already enough intent that could consider target esnext and let the user choose the target they want by letting babel (or some other tool) transpile dependencies (or just otel's) to whatever target they want (be it es2016, es2020, browserlist's defaults, not ie or >1%, or @babel/preset-env's targets.esmodules)

(There ofc may be tooling limitations, I don't know if vite supports transpiling dependencies as it only uses babel for legacy bundles that you only get via a plugin)

While I can't find a good list of what gets transformed with different typescript targets, compat-table has pretty good list of language features by es2016+ version. Out of these I've seen for sure in otel codebase:

@legendecas
Copy link
Member

@t2t2: Why stop at just target: es6? If there is an option for modern-er code that the user has to explicitly opt into, then there's already enough intent that could consider target esnext and let the user choose the target they want

I'm supportive of targeting esnext.

@dyladan
Copy link
Member

dyladan commented Oct 27, 2021

I would be supportive of targeting esnext as an opt-in

@dyladan
Copy link
Member

dyladan commented Nov 22, 2021

@echoontheway can i get you to fix the conflicts so this can be merged?

@legendecas
Copy link
Member

@dyladan I would be supportive of targeting esnext as an opt-in

The new entry point (either "esm2015" or "esnext") is already an opt-in option as users have to explicitly set the entry point to get it recognized by the bundler. Can you elaborate on this?

@dyladan
Copy link
Member

dyladan commented Nov 29, 2021

@dyladan I would be supportive of targeting esnext as an opt-in

The new entry point (either "esm2015" or "esnext") is already an opt-in option as users have to explicitly set the entry point to get it recognized by the bundler. Can you elaborate on this?

I was just voicing my support for the PR and making it clear to readers that it is opt-in for those that don't know.

@dyladan
Copy link
Member

dyladan commented Dec 10, 2021

@echoontheway can you resolve the conflicts?

@echoontheway
Copy link
Contributor Author

@echoontheway can you resolve the conflicts?

resolved, sorry for late.

@vmarchaud vmarchaud added the enhancement New feature or request label Dec 11, 2021
@vmarchaud vmarchaud merged commit b804d9b into open-telemetry:main Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants