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

Script imports are modules by default #22159

Closed
2 tasks done
artu-ole opened this issue Nov 12, 2021 · 16 comments
Closed
2 tasks done

Script imports are modules by default #22159

artu-ole opened this issue Nov 12, 2021 · 16 comments

Comments

@artu-ole
Copy link

artu-ole commented Nov 12, 2021

🐞 Bug report

Command (mark with an x)

  • build
  • serve

Is this a regression?

Yes, the previous version in which this bug was not present was: 12.2.13

Description

Now that dynamic imports are of type="module" by default this breaks imports of legacy libraries that were not built with strict mode in mind.
We are using calentim datepicker in a lazy loaded module(also loading the library with dynamic import) in the project and with the transition to angular 13 and specifically with the change of default webpack output to produce script tags with the type module in f53bf9d the library breaks as it is not written in strict mode.
I understand the desire to move to modules by default, but would like to have an option to import legacy code as 'text/javascript'.
This feels like a breaking change that didn't get a mention in https://update.angular.io notes either.

🔬 Minimal Reproduction

await import('./legacy/library.js')
Using the library will brake during runtime if it violates strict mode.

🌍 Your Environment

ng version



Angular CLI: 13.0.2
Node: 16.13.0
Package Manager: npm 8.1.0
OS: win32 x64

Angular: 13.0.1
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router, service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1300.2
@angular-devkit/build-angular   13.0.2
@angular-devkit/core            13.0.2
@angular-devkit/schematics      13.0.2
@angular/cli                    13.0.2
@schematics/angular             13.0.2
rxjs                            6.6.7
typescript                      4.4.4

Anything else relevant?
I understand the ability to provide custom builder in angular.json, but would like to avoid it as it is a large undertaking to keep it updated between angular releases than being able to configure this some other way.

@ngbot ngbot bot added this to the needsTriage milestone Nov 15, 2021
@alan-agius4 alan-agius4 added needs: discussion On the agenda for team meeting to determine next steps and removed needs: discussion On the agenda for team meeting to determine next steps labels Nov 15, 2021
@dgp1130
Copy link
Collaborator

dgp1130 commented Nov 15, 2021

Hi @artu-ole, thanks for bringing up this change. To give some more context, Webpack downlevels dynamic import() expressions into <script /> tag injections onto the page. Previously this didn't support explicitly stating type="module", but this was recently added to Webpack and enabled in v13, which is where this change came from. Ideally it would have always been type="module", but Webpack just didn't support that at the time.

As for non-module support here, this is an API that actually requires ES modules natively. Loading a non-module via dynamic import() doesn't work natively and we don't want to diverge too much from the web platform in this regard. We have a polyfill which attempts to emulate native behavior, but it is imperfect and unfortunately had this gap in previous CLI versions. Closing this gap makes future migrations to native dynamic import() that much easier.

As for how to handle your particular use case in v13, I think your options are:

  1. Migrate the script to support strict mode.
    • Looks like a third-party library, so probably not something you can directly change, but you can escalate to the relevant owners.
    • Strict mode has been around since 2009 (ES5), so it's pretty reasonable to ask that any currently supported JS is strict mode-compatible.
  2. Add the script to index.html.
    • You can include async and defer, to keep lazy loading it, though you will have less control over how it gets loaded.
  3. Add the script to angular.json.
    • Functionally equivalent to the previous point, but it will be bundled for you.
    • Doesn't seem necessary here, but could be useful in situations like this.

This probably should be documented as part of the release notes, I'll make a PR to update that.

@artu-ole
Copy link
Author

Thank you, @dgp1130, for a comprehensive explanation! We'll evaluate these options and act accordingly. Now that it will be mentioned in the notes is good enough for this to be resolved.

@andreialecu
Copy link
Contributor

This is the only issue I found related to this subject, apologies to hijack it.

We recently hit a critical breaking change with Angular 13 and older SmartTV browsers. We have some customers using Samsung TVs from 2018. Those TVs use Chromium 57 and have not had an update to the browser version, even though they had firmware updates as recent as 2021.

Chrome 57 is from before the time of script type="module", and we already updated our app - not seeing anything concerning in the list of breaking changes (except dropping support for IE 11, but that didn't seem relevant).

Is there any way to force Angular 13 to create normal/non-module scripts and script tags?

Or at least, could you point me to where this change was made? I'd be interested in trying to revert it in a fork so we can get unblocked.

@alan-agius4
Copy link
Collaborator

@andreialecu, Angular only supports the latest major version of Chrome.

See https://angular.io/guide/browser-support for more information about browser support..

The change that introduced this behaviour is #21484

@andreialecu
Copy link
Contributor

andreialecu commented Nov 17, 2021

@alan-agius4 that was also valid for v12: https://v12.angular.io/guide/browser-support

Since Angular is more enterprise focused than other frameworks, I assumed that more care would be taken with listing breaking changes of this sort.

I'd suggest being more careful about these sort of changes, because they can impact Kiosk-like apps where it's impossible to update the browser version. This has already been a problem before: angular/angular#20756 (comment)

Thank you for pointing to the PR, seems like we might be able to get unblocked by post-processing the html file to remove type="module" from script tags.

@alan-agius4
Copy link
Collaborator

@andreialecu, because the app at a certain point works in a particular version of the browser, it doesn’t mean that it is supported unless it’s explicitly listed in the above mentioned page.

Chrome 57 was released back in March 2017, this means that Angular 4 is the last version that officially supported the mentioned browser.

Currently in version 13, only Chrome 96+ is officially supported.

@dgp1130
Copy link
Collaborator

dgp1130 commented Nov 17, 2021

Just to reinforce @alan-agius4's point, we only support relatively recent versions of modern browsers, and Chrome 57 is well outside that range. Things may continue to work by chance, but could break at any time. We don't have the resources to test and verify older versions of browsers to guarantee long term compatibility, especially when it comes to managing other dependencies with similar support levels.

@andreialecu
Copy link
Contributor

Yes, that is totally understandable.

I just thought dropping support for modules would be something that would be important to mention in the change log.

Would have appreciated a warning. 🙂

Otherwise, I agree with browser support approach.

@dgp1130
Copy link
Collaborator

dgp1130 commented Nov 17, 2021

angular/angular#44181 should update the docs to explicitly state this.

FWIW, we didn't drop support for anything, dynamic import() as a native API only supports ES modules. The fact that this was possible without modules before was simply a gap in our implementation which was patched up in v13.

@andreialecu
Copy link
Contributor

That is correct, and as I mentioned, totally understandable.

As long as the community is made aware of what the breaking changes are, most can probably be fixed via polyfills.

<script type="module"> cannot easily be polyfilled and it just results in a blank white page because javascript is not executed at all.

Perhaps the Angular team could keep in mind some of these pinned browsers (to at least warn users on big breaking changes). Personally, we're in a (insignificant) minority of users creating apps that run on Smart TVs. Only a very small number of our users are affected here, because on newer SmartTVs everything is fine.

Also since JS is not ran at all, instrumentation/analytics didn't warn us about anything wrong.

Here's a list of Chrome versions for popular Smart TVs for reference:

https://developer.samsung.com/smarttv/develop/specifications/web-engine-specifications.html
https://webostv.developer.lge.com/discover/specifications/web-engine/

As it is, Angular 13 will only work on LG TVs from 2020 and beyond. Since apps on the LG TV store are made with HTML, any Angular app on the LG TV store upgrading to Angular 13 would get broken. (Hopefully QA can catch this early though).

@andreialecu
Copy link
Contributor

andreialecu commented Nov 18, 2021

diff --git a/src/utils/index-file/augment-index-html.js b/src/utils/index-file/augment-index-html.js
index bf7b2d2fcc3137c80ee222aafb5514d4ddedcf7d..99f0b33cfec79f6bb0708c3df05f2aa9447aceb7 100644
--- a/src/utils/index-file/augment-index-html.js
+++ b/src/utils/index-file/augment-index-html.js
@@ -45,7 +45,8 @@ async function augmentIndexHtml(params) {
     for (const [src, isModule] of scripts) {
         const attrs = [`src="${deployUrl}${src}"`];
         // This is also need for non entry-points as they may contain problematic code.
-        if (isModule) {
+        // disabled because we need support for Chrome 60 and below (LG/Samsung TVs)
+        if (/*isModule*/ false) {
             attrs.push('type="module"');
         }
         else {
diff --git a/src/webpack/configs/browser.js b/src/webpack/configs/browser.js
index 703c7cdf06fe01cce5e76cead3f5e2e8e2bf3a4b..e91140d9073461c10f2a1b3bb9bf9126b97051a3 100644
--- a/src/webpack/configs/browser.js
+++ b/src/webpack/configs/browser.js
@@ -40,7 +40,8 @@ function getBrowserConfig(wco) {
         output: {
             crossOriginLoading,
             trustedTypes: 'angular#bundler',
-            scriptType: 'module',
+            // disabled because we need support for Chrome 60 and below (LG/Samsung TVs)
+            //scriptType: 'module',
         },
         optimization: {
             runtimeChunk: 'single',

If anyone else needs to revert this for whatever reason, the patch above is a quick bandaid for the type="module" problem. Verified everything still works properly for our purposes on older Chrome versions with it.

The diff is using Yarn 3 patch protocol format, but it should be easy to adapt for patch-package if necessary.

Thank you again for pointing to #21484

@ardunster
Copy link

@andreialecu I'm running into a similar issue writing code that runs in an EMR. I have 0 control over the browser that runs it, I don't even know precisely what it is (although html5test.com indicates probably Chrome 57, so that's what I've been targeting). I don't currently do any manual post processing on my build and I'm not sure what file you've changed there. Can you give a little more information?
Thanks!

@andreialecu
Copy link
Contributor

Oh sorry. You can find the actual files by looking them up here: https://github.com/angular/angular-cli/pull/21484/files

I'm currently on my phone and I don't have anything specific to point at.

You can use https://www.npmjs.com/package/patch-package afterwards to auto apply the patch.

@ardunster
Copy link

Nice, thanks, I'll check that out.

@ardunster
Copy link

@andreialecu Thanks again for the heads up on this process. Don't know where my head was Friday, I was searching entirely in the wrong places, got it working in about 15 minutes this morning. My patch is almost identical to yours except the paths are a bit longer. If anyone else is looking at patching this, both of the modified files are inside the src folder inside @angular-devkit/build-angular when installed by npm.

@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 Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants