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

fix: lower target to support Webpack 4 #1085

Merged
merged 4 commits into from
Jun 18, 2022

Conversation

JessicaSachs
Copy link
Contributor

@JessicaSachs JessicaSachs commented Jun 17, 2022

This PR would fix #1073 and target es2019 instead of dropping support for Webpack 4 (which I don't think we should do -- a lot of users are still stuck on Webpack 4, despite how long it has been out).

We're mainly having issues because we use nullish coalescing internally. This requires a target of es2019 instead of es2020. https://esbuild.github.io/content-types/#javascript

Also would fix cypress-io/cypress#21874

@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #1085 (6ffbedd) into main (6d3f42e) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files        2135     2135              
  Lines      229241   229241              
  Branches      980      978       -2     
==========================================
- Hits       228432   228427       -5     
- Misses        788      793       +5     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/finance/index.ts 99.31% <0.00%> (-0.69%) ⬇️
src/modules/internet/user-agent.ts 86.37% <0.00%> (-0.58%) ⬇️

@import-brain import-brain added c: bug Something isn't working p: 1-normal Nothing urgent labels Jun 17, 2022
@import-brain import-brain added this to the v7 - Current Major milestone Jun 17, 2022
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

We should add a CI run for node14.6 or at least the lowest target we support
See also #1052 for my own tries

scripts/bundle.ts Outdated Show resolved Hide resolved
scripts/bundle.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

For tomorrows meeting: I personally find it totally weird that we need to support >2 years old out of date packages, and even if this lowers our target I lost trust in esbuild a bit, because of evanw/esbuild#2315
They are saying that it was because we used esno/tsx, but even now we build it to use native js and it produced the same result in my other branch 🤷
I also don't like that we cannot see which tools and packages out there in the world are possibly not compatible to es20XX and which not. We have a potential table about node versions, but from where can we find out which tools support version xy?
That said, we said that we do not want to support node v12 because it got EOL and so node v14 is the latest "legacy" LTS and it fully supports es2020 🤷
I mean, it is just weird that folks depend on webpack v4 that is out of date, don't get updates (as far as I know) and is not compatible to modern syntax anymore
Is webpack v4 now the new IE11 or what?!

... oops, this turned into a little rant ... 😅

@Shinigami92 Shinigami92 marked this pull request as ready for review June 18, 2022 13:59
@Shinigami92 Shinigami92 requested a review from a team as a code owner June 18, 2022 13:59
@Shinigami92 Shinigami92 added the s: accepted Accepted feature / Confirmed bug label Jun 18, 2022
@Shinigami92 Shinigami92 changed the title fix: lower target to support Webpack 4 (close #1073) fix: lower target to support Webpack 4 Jun 18, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) June 18, 2022 14:04
@JessicaSachs
Copy link
Contributor Author

Shini and I spoke. We'll be supporting es2019 + Webpack 4.

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

I totally do not understand how this works but if you ensure that it does I'm fine with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
4 participants