Skip to content

fix(@angular-devkit/build-angular): downlevel libraries based on the browserslist configurations #23185

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

Merged
merged 1 commit into from
May 24, 2022

Conversation

alan-agius4
Copy link
Collaborator

There is no standard for library authors to ship their library in different ES versions, which can result in vendor libraries to ship ES features which are not supported by one or more browsers that the user's application supports.

With this change, we will be downlevelling libraries based on the list of supported browsers which is configured in the browserslist configuration. Previously, we only downlevelled libraries when targeting ES 5.

The TypeScript target option will not effect how the libraries get downlevelled.

Closes #23126

@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from 2584193 to 5732080 Compare May 20, 2022 09:29
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from 5732080 to e6ea05a Compare May 20, 2022 09:37
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from e6ea05a to c71832f Compare May 20, 2022 09:49
@alan-agius4 alan-agius4 reopened this May 20, 2022
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from b033dd2 to 3bbc289 Compare May 20, 2022 10:04
@alan-agius4 alan-agius4 requested a review from clydin May 23, 2022 12:47
@alan-agius4 alan-agius4 marked this pull request as ready for review May 23, 2022 12:47
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from 3bbc289 to 93c45fc Compare May 23, 2022 12:48
@alan-agius4 alan-agius4 added the target: rc This PR is targeted for the next release-candidate label May 23, 2022
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from 93c45fc to 86c1e91 Compare May 23, 2022 17:09
@dgp1130 dgp1130 added this to the v14 milestone May 23, 2022
@alan-agius4 alan-agius4 requested a review from dgp1130 May 23, 2022 17:53
dgp1130
dgp1130 previously approved these changes May 23, 2022
Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

Commit message nit: "fix(@angular-devkit/build-angular): downlevel libraries based on the browserlist configuration"

@alan-agius4 alan-agius4 changed the title fix(@angular-devkit/build-angular): downlevelled libraries based on the browserlist configurations fix(@angular-devkit/build-angular): downlevel libraries based on the browserlist configurations May 24, 2022
@alan-agius4 alan-agius4 changed the title fix(@angular-devkit/build-angular): downlevel libraries based on the browserlist configurations fix(@angular-devkit/build-angular): downlevel libraries based on the browserslist configurations May 24, 2022
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch 2 times, most recently from 7feedfa to bfe5af8 Compare May 24, 2022 06:28
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label May 24, 2022
…browserslist configurations

There is no standard for library authors to ship their library in different ES versions, which can result in vendor libraries to ship ES features which are not supported by one or more browsers that the user's application supports.

With this change, we will be downlevelling libraries based on the list of supported browsers which is configured in the browserslist configuration. Previously, we only downlevelled libraries when targeting ES5.

The TypeScript target option will not effect how the libraries get downlevelled.

Closes angular#23126
@alan-agius4 alan-agius4 force-pushed the preset-env-use-browserslist branch from bfe5af8 to 5df577b Compare May 24, 2022 14:31
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 24, 2022
@dgp1130 dgp1130 merged commit cd2250f into angular:main May 24, 2022
@alan-agius4 alan-agius4 deleted the preset-env-use-browserslist branch May 24, 2022 17:03
@terencehonles
Copy link

I'm a bit confused after this change.

I was hoping it would just work, but it doesn't seem like my browser list is being respected. I have it defined in the package.json and I'm not sure if that might be the issue.

I previously had the tsconfig set to es5 to output for legacy browsers, and I had a check to make sure the built bundle was es5 to catch any libraries that snuck in which is what this change was meant to address.

After upgrading to Angular 14 the migration changed my tsconfig output setting, and since I had previously seen this change I figured it was fine and would just work. However, the es5 check is now complaining and the only way to make sure the build is correct is to set the target back to es5 which triggers the warning that es5 output is deprecated.

Is there some configuration option I am missing? I can obviously set the target back to es5 but I'm not sure why there was a migration to change it if this breaks now.

Thanks in advance and I can open an issue if needed.

@alan-agius4
Copy link
Collaborator Author

Indeed ES5 output support is deprecated since Angular no longer supports legacy browsers such as IE 11 which require ES5 syntax. I am not really sure what you mean by the es5 check is now complaining.

Without more information such as a minimal reproduction it's hard wether or not there is an issue.

@terencehonles
Copy link

terencehonles commented Jun 8, 2022

@alan-agius4 I may want to just open an issue if more details are needed than this, but the ES5 check I am referring to is using https://github.com/acornjs/acorn after the build step to check if any libraries had slipped in non ES5 features since libraries weren't previously downleveled before this change. Previously our tsconfig output was set to es5, but after updating to Angular 14 and migrating, it changed to es2020.

Based on the title of this PR, and what I thought the feature would do, is that the tsconfig output could now be set to es2020 which was previously used internally and all generated output would respect the browserslist configuration. I don't need to strictly check for es5 and I can remove acorn from our build process, but I did notice that the output generated was still using features that were too new for our browserslist configuration and I'm wondering if I'm either not using an option I should be or if I'm misunderstanding what's supposed to happen and I should keep the tsconfig output at es5 until we can remove our need for that downleveling.

@alan-agius4
Copy link
Collaborator Author

Hi @terencehonles,

During the update to version 14 we do indeed update the target to ES2020, this is because all browsers that Angular supports ES2020 features out of the box without the need of any downlevelling or polyfilling features.
Out of curiosity, why do you have requirement to ship ES5? ES5 is larger in terms of bundle size.

Prior to this change downlevelling of libraries was only done when targetting ES5. Now downlevelling for libraries always happen. That said this change doesn't effect the TypeScript output when targetting ES2015 or later. Downlevelling of TypeScript sources is only done when targeting ES5 and this is done because various parts of the build only support modern ES syntax.

Moving forward in another major version we want to change this and "ignore" the TypeScript target and downlevel ECMA features/syntax based on the configured browserslist configuration even on TypeScript output.

@terencehonles
Copy link

terencehonles commented Jun 9, 2022

Moving forward in another major version we want to change this and "ignore" the TypeScript target and downlevel ECMA features/syntax based on the configured browserslist configuration even on TypeScript output.

Ah, ok, so reverting the migration does seem like the correct course of action for us. I thought this was already in effect and why I was commenting.

Out of curiosity, why do you have requirement to ship ES5? ES5 is larger in terms of bundle size.

We unfortunately need ES5 because we currently use wkhtmltopdf to generate PDFs of our pages and it's based on QtWebKit ~= Safari 5. We've not had the time to invest in changing that part of our stack and we're "OK" with everyone seeing ES5 for the time being 😞. We've made progress in cleaning up those pages so that they can be rendered with something like puppeteer instead, but unfortunately all of that work has just been on "spare time" and was never a priority compared to other things we needed to ship.

Previously, the v8 differential loading feature was a big factor in why we were "OK" with still having wkhtmltopdf. Our polyfills were probably a bit bloated at that point, but it was something we could live with since we were still in the progress of adopting Angular and our project wasn't that big. Over time more of the application has been converted and we've cleaned up things needed in order to drop super old browsers, but we will still want the ability to target browsers that Angular may not support and that's where we are definitely looking forward to a more comprehensive down leveling step built into the CLI.

Thanks for all your responses!

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different handling of EcmaScript version specific features in JS and TS code
4 participants