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

build: set target to "es2021" #3840

Merged
merged 2 commits into from
Sep 19, 2023
Merged

build: set target to "es2021" #3840

merged 2 commits into from
Sep 19, 2023

Conversation

justinmk3
Copy link
Contributor

@justinmk3 justinmk3 commented Sep 19, 2023

Problem:
"es6" is very old and all of our targets support es2021. Targeting es6 disallows new javascript features such as Promise.allSettled().

Solution:

  • increase target to "es2021".
  • improve logging
  • invalidate(): use Promise.allSettled() instead of all()

No significant change in (unminified) artifact size:

:!du -hs */dist/src/main.js  
 33M    es2021/dist/src/main.js                                                                                                                                                                                                   
 33M    es6/dist/src/main.js

Sample of webpack'd source changes:

image

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@justinmk3 justinmk3 requested a review from a team as a code owner September 19, 2023 12:57
Problem:
"es6" is very old and all of our targets support es2021.
Targeting es6 disallows new javascript features such as Promise.allSettled().

Solution:
- increase target to "es2021".
    - also update webpack config (why is this duplicated across configs?)
- improve logging
- invalidate(): use Promise.allSettled() instead of all()
@@ -69,7 +69,7 @@ const baseConfig = {
loader: 'esbuild-loader',
options: {
loader: 'ts',
target: 'es2018',
target: 'es2021',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why isn't this driven by tsconfig...?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was a design choice by the webpack team. There was an edge case where you could use the same webpack config for multiple projects and it wouldn't know which of the projects targets it should use. So they just made it explicitly required.

@@ -69,7 +69,7 @@ const baseConfig = {
loader: 'esbuild-loader',
options: {
loader: 'ts',
target: 'es2018',
target: 'es2021',
Copy link
Contributor

Choose a reason for hiding this comment

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

It was a design choice by the webpack team. There was an edge case where you could use the same webpack config for multiple projects and it wouldn't know which of the projects targets it should use. So they just made it explicitly required.

@justinmk3
Copy link
Contributor Author

justinmk3 commented Sep 19, 2023

The failing Insiders CI jobs are unrelated to this PR and are likely an upstream issue in VSCode. Same failures happen on other PRs.

  1) "before each" hook: beforeEach for "returns a webview with tokens replaced":
     Uncaught TypeError: Cannot read properties of null (reading 'finishWrite')

@justinmk3 justinmk3 enabled auto-merge September 19, 2023 14:18
@justinmk3 justinmk3 disabled auto-merge September 19, 2023 14:19
@justinmk3 justinmk3 merged commit cb07bb2 into master Sep 19, 2023
5 of 8 checks passed
@justinmk3 justinmk3 deleted the logger branch September 19, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants