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.

Sign up for GitHub

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

fix: enable downlevelIteration for es5 targets #2823

Merged
merged 3 commits into from
Mar 22, 2022
Merged

fix: enable downlevelIteration for es5 targets #2823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dae0e56
chore: run additional compilation variants
legendecas Mar 9, 2022
e65b0a9
Merge branch 'main' into metrics-ff/ci
legendecas Mar 21, 2022
de6ba47
Merge branch 'main' into metrics-ff/ci
vmarchaud Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/unit-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ jobs:
- name: Build 🔧
run: |
npm run compile
# run additional compilation variants
Copy link
Member

Choose a reason for hiding this comment

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

This is going to duplicate compile many many times because each package you run compile in also compiles its dependencies. This is why the compile is done from npm run compile to ensure everything is only compiled a single time.

Copy link
Member Author

Choose a reason for hiding this comment

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

The time spent is about 6minutes compared to about 2minutes before. And the full-compilation only happens in browser-tests -- Node.js tests only run the root tsconfig.json build.

Since these products are going to be released, we need to make sure the releasing variant is working so I think this is acceptable, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

ah sorry I thought I already responded here. I think it's fine.

npx lerna run compile

- name: Unit tests
run: npm run test:browser
Expand Down Expand Up @@ -111,6 +113,8 @@ jobs:
- name: Build 🔧
run: |
npm run compile
# run additional compilation variants
npx lerna run compile

- name: Unit tests
run: npm run test:webworker
Expand Down Expand Up @@ -192,6 +196,8 @@ jobs:
working-directory: experimental
run: |
npm run compile
# run additional compilation variants
npx lerna run compile

- name: Unit tests
working-directory: experimental
Expand Down Expand Up @@ -231,6 +237,8 @@ jobs:
working-directory: experimental
run: |
npm run compile
# run additional compilation variants
npx lerna run compile

- name: Unit tests
working-directory: experimental
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.es5.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"target": "es5"
"target": "es5",
"downlevelIteration": true
Copy link
Member Author

@legendecas legendecas Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Though, Map/Set is available since ES2015. TypeScript doesn't introduce any polyfills for Map/Set. I think the es5-esm products we are publishing right now do not actually work for es5 environments without specific setups.

The good news is that the es5-esm products do not contain any fashion syntaxes (other than the ESM part) so that end-users do not need to transpile the otel-packages. They just need to include the polyfill packages. I believe this doesn't break the assumptions #2472 (comment).

}
}