Remove System.import() usage in favor of import()

@hansl

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

Currently Angular core makes use of a System.import() call to load templates (html/css) and lazy modules:

angular/packages/core/src/linker/system_js_ng_module_factory_loader.ts

Lines 67 to 70 in 3bcc0e6

    
           return System.import(module) 
        
               .then((module: any) => module[exportName]) 
        
               .then((type: any) => checkNotEmpty(type, module, exportName)) 
        
               .then((type: any) => this._compiler.compileModuleAsync(type));

In the past System.import was expected to become a standard, but import() was settled upon instead.

Modern tooling is beginning to deprecate (notably Webpack 4 webpack/webpack#6321) System.import() detection in favor of the standard and it should not be unexpected that more follow suit.

Expected behavior

The standard import() is used, so that tooling that supports code splitting can detect it.

Minimal reproduction of the problem with instructions

The SystemJS loader can be found at https://github.com/angular/angular/blob/master/packages/core/src/linker/system_js_ng_module_factory_loader.ts.

What is the motivation / use case for changing the behavior?

Remain compatible with modern tooling.

Environment


Angular version: 2, 4 and 5

/cc @hansl @bradlygreen @IgorMinar @robwormald

In theory I'm all for this, but what happens with ES5 code? What does import() transpile down to do for a UMD or CommonJS bundle? It doesn't seem like this is something we can do in a backwards compatible fashion, unless I'm missing something.

I'd prefer to drop this whole abstraction in favor of just doing it inline:

{ loadChildren: import('./some-child').then(mod => mod.SomeChild) }

but that's going to be gated on inlining factories (so we don't need to do the rewrite), or making CLI smart enough to pick up imports like ^ and rewrite them.

Personally I'd prefer dropping the abstraction as well, but haven't considered the implications of that option much.

The CLI could be smart enough to pick up those imports I think. We already have to do that for System.import.

It was my understanding that import() was only available with when targeting es2015 as a requirement of TS, meaning it does not support es5.

It's not a big problem for applications proper. It's more of an issue for libraries and packaging concerns. If Angular were to have import() in it's codebase, it could only be consumed by tooling that understood import() and know to also load what the statement was referencing.

On formats like UMD or CommonJS it would have to be provided externally. I don't think it transpiles down to anything.

@Toxicable

@Toxicable Dynamic import has no restriction on target, but on module. All module types except es2015 are allowed, for example with source:

export function load(path) {
    return import(path);
}

To module: esnext:

export function load(path) {
    return import(path);
}

To module: commonjs:

"use strict";
exports.__esModule = true;
function load(path) {
    return Promise.resolve().then(function () { return require(path); });
}
exports.load = load;

To module: amd:

define(["require", "exports"], function (require, exports) {
    "use strict";
    exports.__esModule = true;
    function load(path) {
        return new Promise(function (resolve_1, reject_1) { require([path], resolve_1, reject_1); });
    }
    exports.load = load;
});

To module: system:

System.register([], function (exports_1, context_1) {
    "use strict";
    var __moduleName = context_1 && context_1.id;
    function load(path) {
        return context_1["import"](path);
    }
    exports_1("load", load);
    return {
        setters: [],
        execute: function () {
        }
    };
});

To module: umd:

(function (factory) {
    if (typeof module === "object" && typeof module.exports === "object") {
        var v = factory(require, exports);
        if (v !== undefined) module.exports = v;
    }
    else if (typeof define === "function" && define.amd) {
        define(["require", "exports"], factory);
    }
})(function (require, exports) {
    "use strict";
    var __syncRequire = typeof module === "object" && typeof module.exports === "object";
    exports.__esModule = true;
    function load(path) {
        return __syncRequire ? Promise.resolve().then(function () { return require(path); }) : new Promise(function (resolve_1, reject_1) { require([path], resolve_1, reject_1); });
    }
    exports.load = load;
});

@trotyl

@trotyl Right, my mistake I confused the two of them.
However, this would still be a breaking change since most people use module: "es2015" since that's the CLI's default

@Toxicable

@Toxicable Just did a check, the module restriction is only meant for .ts source files (compilation), not for .d.ts declaration files (type-checking).

The emited .d.ts is:

export declare function load(path: string): Promise<any>;

There's no pattern indicating the dynamic import.

Oh that's an interesting point maybe it would be fine to make that change then

@trotyl

Great breakdown @trotyl!

@filipesilva

@filipesilva will this be still compatible with webpack 3?

@IgorMinar

@IgorMinar Webpack 3 supports import() as the primary code splitting construct https://webpack.js.org/guides/code-splitting/#dynamic-imports.

System.import() isn't listed in the docs even, but it is supported since the Webpack 3 betas.

FWIW, I tried { loadChildren: import('./some-cool-module').then(mod => mod.ModuleName) } and it seems to work in JIT mode, but I had to switch to { loadChildren: './path-to-module#ModuleName' } to get it to work with AOT.

Was using Angular 5.x.x and Webpack 3.x.x.

If import was used in Angular, would that mean that we could code split in more places than just the routes and still have AOT work? That would be amazing. Thinking the equivalent of something like the react-loadable project where component level splitting was an option without having to tie the split to the router.

In Angular 8 this warning wasn't resolved

@ermarkar

Please, see the comment by @ermarkar #23426 (comment)

@almartino

Good to know how to hide the warning, @almartino , but the team really should move away from SystemJS imports soon. It's practically 2020 and we have lots of support for native import().

In Angular 9 this warning wasn't resolved :'( but using what PKief suggested worked for me with webpack 4

{
    test: /\.js$/,
    parser: { system: false },
}

Can anyone care to explain what is the downside of disabling the SystemJS other than freeing the global variable "System"?

Quoting from Performance section

SystemJS is designed for production modules performance and can load multiple modules in less than a millisecond. Its performance is only around a factor of 1.5 times the performance of native ES modules, as seen in the following performance benchmark, which was run by loading 426 javascript modules (all of @babel/core) on a Macbook pro with fast wifi internet connection. Each test was the average of five page loads in Chrome 80.

Facing this issue with "@angular/core": "^11.2.10",

@rahulserver

@rahulserver - which issue exactly are you facing? This is a feature request.

@petebacondarwin

@petebacondarwin I just imported that angular core library in an ionic project (Which is actually a vue project) because I had to us a barcode scanner module https://ionicframework.com/docs/native/barcode-scanner (which needs angular/core in a vue js project).
And I get this in console whenever i run the project:


 warning  in ./node_modules/@angular/core/fesm2015/core.js

Critical dependency: the request of a dependency is an expression

 warning  in ./node_modules/@angular/core/fesm2015/core.js

System.import() is deprecated and will be removed soon. Use import() instead.
For more info visit https://webpack.js.org/guides/code-splitting/

 @ ./node_modules/@ionic-native/barcode-scanner/ngx/index.js 2:0-43 24:16-26
 @ ./node_modules/cache-loader/dist/cjs.js??ref--12-0!./node_modules/babel-loader/lib!./node_modules/cache-loader/dist/cjs.js??ref--0-0!./node_modules/vue-loader-v16/dist??ref--0-1!./src/components/modals/NewPurchaseModal.vue?vue&type=script&lang=js
 @ ./src/components/modals/NewPurchaseModal.vue?vue&type=script&lang=js
 @ ./src/components/modals/NewPurchaseModal.vue
 @ ./node_modules/cache-loader/dist/cjs.js??ref--12-0!./node_modules/babel-loader/lib!./node_modules/cache-loader/dist/cjs.js??ref--0-0!./node_modules/vue-loader-v16/dist??ref--0-1!./src/views/Request.vue?vue&type=script&lang=js
 @ ./src/views/Request.vue?vue&type=script&lang=js
 @ ./src/views/Request.vue
 @ ./src/router/index.js
 @ ./src/main.js
 @ multi (webpack)-dev-server/client?http://192.168.0.100:8080&sockPath=/sockjs-node (webpack)/hot/dev-server.js ./src/main.js

So the issue is that you are receiving a warning due to this deprecated System.import call in the code.
This will be removed in the next major release (v13 since 12.0.0 is about to ship next week).

SystemJsNgModuleFactoryLoader has been removed in #43591.

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.}

Navigation Menu

Repository navigation

Remove `System.import()` usage in favor of `import()` #21560

I'm submitting a...

Current behavior

Expected behavior

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

Environment

28 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	return System.import(module)
	.then((module: any) => module[exportName])
	.then((type: any) => checkNotEmpty(type, module, exportName))
	.then((type: any) => this._compiler.compileModuleAsync(type));

Remove System.import() usage in favor of import() #21560

Description

I'm submitting a...

Current behavior

Expected behavior

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

Environment

Activity

robwormald commented on Jan 16, 2018

filipesilva commented on Jan 16, 2018

Toxicable commented on Jan 16, 2018

filipesilva commented on Jan 16, 2018

trotyl commented on Jan 17, 2018

Toxicable commented on Jan 17, 2018

trotyl commented on Jan 17, 2018

Toxicable commented on Jan 17, 2018

filipesilva commented on Jan 17, 2018

IgorMinar commented on Jan 20, 2018

filipesilva commented on Jan 29, 2018

dennis-johnson-dev commented on Mar 2, 2018

28 remaining items

wKoza commented on Aug 5, 2019

almartino commented on Dec 20, 2019

thw0rted commented on Dec 30, 2019

iranicus commented on Feb 23, 2020

seahindeniz commented on Aug 9, 2020

rahulserver commented on May 6, 2021

petebacondarwin commented on May 6, 2021

rahulserver commented on May 6, 2021

petebacondarwin commented on May 7, 2021

JoostK commented on Sep 30, 2021

angular-automatic-lock-bot commented on Oct 31, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Remove `System.import()` usage in favor of `import()` #21560