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: target to es2017 in the no-polyfill target #2765

Merged
merged 3 commits into from
Feb 24, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Feb 6, 2022

Which problem is this PR solving?

This is a follow-up of #2556.

As the original PR claims to fix #2471, unfortunately, the problem is not resolved. The async function was introduced with es2017, so targeting es2015 is not enough and it is still been polyfill-ed. #2556 (comment)

This PR aligns the target with tsconfig.base.json (which is used for Node.js targets). Also renamed the target name as "esnext", so that we can freely upgrade the target to a later version of ECMAScript.

Another problem is that referencing projects with tsconfig suffix like ".all.json" in the root tsconfig project is not supported by the update-ts-references yet. The root tsconfig.json is always being updated during the bootstrap since the update-ts-references is invoked with after-install hook. This can be confusing, for git tracked files were updated during bootstrap of the project. These targets are only needed when publishing, so we don't have to compile them on the root project "compile" script. The pre-publish for each package invokes the "compile" script, which is compiled with the "tsconfig.all.json".

Short description of the changes

  1. Target es2017 in the no-polyfill targets.
  2. Rename the no-polyfill targets to "esnext".
  3. Removes the .all.json suffixes in the root tsconfig.json references.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Feb 6, 2022

Codecov Report

Merging #2765 (df319bc) into main (144e11a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2765   +/-   ##
=======================================
  Coverage   93.41%   93.41%           
=======================================
  Files         159      159           
  Lines        5450     5450           
  Branches     1145     1145           
=======================================
  Hits         5091     5091           
  Misses        359      359           

@legendecas legendecas marked this pull request as ready for review February 6, 2022 07:45
@legendecas legendecas requested a review from a team February 6, 2022 07:45
@legendecas legendecas mentioned this pull request Feb 9, 2022
4 tasks
@legendecas legendecas requested a review from a team February 15, 2022 02:39
@dyladan
Copy link
Member

dyladan commented Feb 23, 2022

Can this be considered a breaking change? If someone was previously targeting the es2015 build but now it is removed and esnext is added?

@legendecas
Copy link
Member Author

@dyladan

Can this be considered a breaking change?

We have not released this yet. So I don't think so.

@dyladan
Copy link
Member

dyladan commented Feb 23, 2022

The esm2015 target was released in november. Currently released packages can be built from it I am pretty sure.

edit: looks like I'm wrong. The PR was opened in october but was merged in december and is not released.

This is not a breaking change

@legendecas legendecas merged commit b8df001 into open-telemetry:main Feb 24, 2022
@legendecas legendecas deleted the tsconfig branch February 24, 2022 08:32
@schickling
Copy link

For my projects this in fact ended up being a breaking change as broken builds were published to NPM.

image

image

CleanShot 2022-03-22 at 09 16 28

@legendecas
Copy link
Member Author

@schickling we're working to fix it: #2844. Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM build targets ES5 resulting in polyfills for await
4 participants