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(typescript): upgrade target from es2017 to es2021 #1987

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Jul 14, 2024

Q A
Bug fix? no
New feature? no
Issues Fix #...
License MIT

We are in 2024, but we still targets ES2017 which requires TypeScript to import some helper functions (to handle const {a, ...b} = c and private class methods), which then ship Microsoft license (cc #1937 (comment)).

Since those dist/ files are:

  • either handled by Symfony AssetMapper/ImportMap, which requires browsers to have importmap support (if a browser supports importmap, it supports rest operator and private class methods)
  • either handled by Symfony Webpack Encore, which can polyfill if necessary through Babel/core-js

IMO we can configure an highest TypeScript target. We will have the following benefits:

  • no more Microsoft license
  • no more useless functions calls, so small performance improvments
  • lighter dist files

WDYT?

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Jul 14, 2024
@smnandre
Copy link
Member

Do we need to change it for all packages at once ?

@Kocal
Copy link
Member Author

Kocal commented Jul 14, 2024

Why not? What advantages and disadvantages do we have if we change it for all packages or specific packages?

@smnandre
Copy link
Member

If the generated JS is different (even for a tiny bit), we would need to release a new version, and i’m not fan of new versions without features

@Kocal
Copy link
Member Author

Kocal commented Jul 14, 2024

There is no problem with releasing minor versions 🤔

@smnandre
Copy link
Member

But still .... can we or is it annoying ? it's a genuine question ^^

@WebMamba
Copy link
Collaborator

Humm I am not sure if we can do that. I am think about people using a UX component, in a custom webpack configuration, I think this could break for them.

@@ -1,11 +1,18 @@
import { Controller } from '@hotwired/stimulus';

class default_1 extends Controller {
app;
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why only the controller have changed and not other .js files (example in Svelte folder) ?

tsconfig.json Outdated
@@ -7,7 +7,7 @@
"rootDir": "src",
"strict": true,
"strictPropertyInitialization": false,
"target": "es2017",
"target": "es2022",
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea if what i'm asking there is stupid tbh ^^

But should we update the rest of these options, like

"lib": ["dom", "es2015"],
"module": "es2015",

Copy link
Member Author

@Kocal Kocal Jul 14, 2024

Choose a reason for hiding this comment

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

target already modify lib setting, but module is another setting not related to lib or target

@Kocal
Copy link
Member Author

Kocal commented Jul 14, 2024

They still can run Babel/core-js on node_modules/, but yeah that should be told explicitly.

PS: To me, in an ideal world, JavaScript packages should only contains source files and the user is responsible for building them.

@smnandre
Copy link
Member

PS: To me, in an ideal world, JavaScript packages should only contains source files and the user is responsible for building them.

I think (as of today, no hard stance) the opposite. Well, i think like you and also the opposite 😅
We could provide 3 different things in fact

  • small compiled JS files that anyone can copy-paste if needed, with no external install/tool in the public folder
  • packed module file in dist (as today)
  • source files build / compile / pack / whatever the dev want

That a very intersting debate, probably deserve a new issue for it :)

@smnandre
Copy link
Member

Humm I am not sure if we can do that. I am think about people using a UX component, in a custom webpack configuration, I think this could break for them.

@Kocal can you post here a small objective list of what the impacts are / can be.

Regarding ES2022 i checked on caniuse.com and i'm 100% for, but...

We need prepare this with:

  • small documentation/FAQ file "how to ..." for the babel solution
  • announce it widely (GIT / tweets / message on the website) the first day after the previous release
  • give people a month to check (we merge and people can test the dev version) .... not during the summer where people are not around

I'm not sure if this could be considered BC break ... in a pragmatic sense.

@smnandre
Copy link
Member

That allow me to reintroduce an idea that stayed in my head for too long: we need an "official" / public / clear browser compatibility list.

And i don't think this can be the same as the Symfony php (no one can honestly garantee total browser support for 5 years, and we don't have multiple versions at once for the moment).

That aslo would require an entire debate/issue.

@smnandre
Copy link
Member

Update: i'd like to avoid "static class initialization" as they are not compatible with 2 or 3 years old iPhones :|

Can we pick a subset of features ?

-> https://caniuse.com/mdn-javascript_classes_static_initialization_blocks

@Kocal
Copy link
Member Author

Kocal commented Jul 14, 2024

Can we pick a subset of features ?

I don't really think... :/ Maybe with lib compiler option?

Anyway, if we wan't to keep classes definition as-if in main, we can use es2018 which only keeps { a, ...b } = in a native way.

@smnandre
Copy link
Member

Would ES2020 / ES2021 be good for you ? It seems to match your needs and has a much better support table for now.

@Kocal
Copy link
Member Author

Kocal commented Jul 15, 2024

Good for me, but you will still have the Microsoft license in Autocomplete dist code 😛

@Kocal
Copy link
Member Author

Kocal commented Jul 15, 2024

I've downgraded to ES2020 to keep tslib's helpers for class private members, and finally upgraded to ES2021 (it has no effect on the code yet).

@WebMamba
Copy link
Collaborator

I am not 100% about it! Can we create separate branch for people that wan't to use more recent ES version? And just keep the main branch as it is now, and merge during a major version ?

@Kocal
Copy link
Member Author

Kocal commented Jul 15, 2024

I am not 100% about it!

I mean, for the code actually present in the code base, if I use es2020 or es2021 as target, the same exact dist code is generated

@Kocal Kocal changed the title chore(typescript): upgrade target from es2017 to es2022 chore(typescript): upgrade target from es2017 to es2021 Jul 19, 2024
@Kocal
Copy link
Member Author

Kocal commented Jul 19, 2024

This PR is now using es2021 instead of es2017, and only updates the code to use native rest operator, and is supported by ~95%+ of the browsers.

I believe we can merge this safely :)

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

👏

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jul 19, 2024
@kbond
Copy link
Member

kbond commented Jul 24, 2024

Thanks Hugo.

@kbond kbond merged commit f5b1413 into symfony:2.x Jul 24, 2024
5 checks passed
@Kocal Kocal deleted the chore/ts-target-es2022 branch July 24, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants