Disable System.import parser plugin by default #6321

ooflorent · 2018-01-15T20:34:23Z

What kind of change does this PR introduce?

refactor

Did you add tests for your changes?

yes

If relevant, link to documentation update:

TODO since the parser options are updated.

Summary

System.import is deprecated since version 2.1.0-beta.28. ~~This PR disables System.import by default. Previously its support was opt-in. In order to enable it, the parser option system must be enabled:~~

This PR introduce a new warning if System.import() is used. The plugin can be toggled using a parser option:

module: {
  rules: [
    {
      test: /\.js$/,
      parser: {
        system: true // no warning
      }
    }
  ],
}

Possible system values:

default: Consider System.import() as import(). Emit a warning if System.import() is used.
true: Consider System.import() as import(). No warning.
false: System.import() is not intercepted. Using it most like triggers a TypeError

Does this PR introduce a breaking change?

yes (new warning)

Other information

cc @TheLarkInn @hansl @filipesilva

ooflorent · 2018-01-15T20:35:41Z

lib/dependencies/SystemPlugin.js

@@ -39,6 +39,11 @@ class SystemPlugin {
 						parser.state.module.context, require.resolve("../../buildin/system.js"));
 					return ParserHelpers.addParsedVariableToModule(parser, "System", systemPolyfillRequire);
 				});
+
+				parser.hooks.call.for("System.import").tap("SystemPlugin", expr => {
+					parser.hooks.importCall.call(expr);


This trick removes System.import handling from ImportPlugin by simulating an import() call.

ooflorent · 2018-01-15T20:36:02Z

lib/dependencies/SystemPlugin.js

@@ -15,7 +15,7 @@ class SystemPlugin {
 			normalModuleFactory
 		}) => {
 			const handler = (parser, parserOptions) => {
-				if(typeof parserOptions.system !== "undefined" && !parserOptions.system)
+				if(typeof parserOptions.system === "undefined" || !parserOptions.system)


Unlike other parser options, this one is disabled by default.

TheLarkInn · 2018-01-16T04:44:07Z

Will users be warned they are using something that doesn't work anymore? My one concern: people use it thinking there is code splitting and it does nothing. Will this throw now?

sokra · 2018-01-16T08:01:45Z

Will users be warned they are using something that doesn't work anymore? My one concern: people use it thinking there is code splitting and it does nothing. Will this throw now?

Good point.

Maybe we should do this for webpack 4 until webpack 5.

system: true -> support System.import like import()
system: undefined -> support System.import like import() but prints errors
system: false -> ignores System.import

ooflorent · 2018-01-16T08:42:37Z

@sokra working on it 👍

ooflorent · 2018-01-16T08:57:32Z

I've also updated the PR message to describe the new behavior.

ooflorent · 2018-01-16T08:57:50Z

lib/dependencies/SystemPlugin.js

+
+				parser.hooks.call.for("System.import").tap("SystemPlugin", expr => {
+					if(shouldWarn) {
+						shouldWarn = false;


Emit the warning once. Only if System.import() is used somewhere.

filipesilva · 2018-01-16T10:47:35Z

The behaviour described by @sokra works for Angular CLI 👍

ooflorent · 2018-01-16T10:51:01Z

@filipesilva I've implemented it in this PR. The warning will be visible only if System.import is found.

sokra · 2018-01-16T12:53:53Z

lib/dependencies/SystemPlugin.js

+		this.name = "SystemImportDeprecationWarning";
+		this.message = "System.import() is deprecated and will be removed soon.\n" +
+			"Use import() or require.ensure() instead.\n" +
+			"For more info visit https://webpack.js.org/guides/code-splitting/";


This should print a list of all modules using System.import.

What about adding this to the warning and allow multiple emit?

this.origin = this.module = module; this.originLoc = loc;

The above modification emits the following warnings:

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

It may be better to debug these messages rather than a module list. What do you think?

Should I remove will be removed soon? I feel like it's a bit to much.

yep, that's better.

Add the warning to parser.scope.module.warnings instead of to the Compilation directly. This should ensure that it's correctly cached.

Stats does not extract originLoc of Module#warnings; only Compilation#warnings. I'm unable to match the location into warnings.js when using parser.scope.module instead of compilation.

I thought Module#warnings are added to Compilation#warnings after parsing...

sokra · 2018-01-16T12:54:12Z

lib/node/NodeMainTemplatePlugin.js

@@ -36,7 +36,7 @@ module.exports = class NodeMainTemplatePlugin {
 					`${mainTemplate.requireFn}.oe = function(err) {`,
 					Template.indent([
 						"process.nextTick(function() {",
-						Template.indent("throw err; // catch this error by using System.import().catch()"),
+						Template.indent("throw err;"),


// catch this error by using import().catch()

sokra · 2018-01-16T12:54:58Z

test/HotTestCases.test.js

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


Remove all System.import calls in the hot test cases.

sokra · 2018-01-16T12:55:23Z

test/TestCases.test.js

@@ -159,6 +159,11 @@ describe("TestCases", () => {
 								},
 								module: {
 									rules: [{
+										test: /\.js$/,
+										parser: {
+											system: true


Remove all System.import calls in the test cases.

sokra · 2018-01-16T12:56:55Z

test/configCases/target/node-dynamic-import/warnings.js

@@ -0,0 +1,3 @@
+module.exports = [
+	[/System\.import\(\) is deprecated/]
+];


Add additional config testcases for system: true,system: undefined and system: false

sokra · 2018-01-16T12:57:52Z

lib/dependencies/SystemPlugin.js

+						compilation.warnings.push(new SystemImportDeprecationWarning());
+					}
+
+					parser.hooks.importCall.call(expr);


return the result of this line

sokra · 2018-01-16T12:58:19Z

lib/dependencies/SystemPlugin.js

+
+		this.name = "SystemImportDeprecationWarning";
+		this.message = "System.import() is deprecated and will be removed soon.\n" +
+			"Use import() or require.ensure() instead.\n" +


remove require.ensure(), only recommend import()

sokra · 2018-01-18T08:35:59Z

test/cases/parsing/typeof/index.js

@@ -29,13 +29,6 @@ it("should answer typeof require.ensure correctly", function() {
 it("should answer typeof require.resolve correctly", function() {
 	(typeof require.resolve).should.be.eql("function");
 });
-it("should answer typeof System correctly", function() {


These two tests should be moved into configCases/parsing/system.import

sokra · 2018-01-18T08:37:24Z

test/configCases/parsing/system.import/index.js

+	try {
+		System.import("./module").then(mod => {
+			mod.should.be.eql({ default: "ok" });
+			done();


This should error here when __SYSTEM === false

sokra · 2018-01-18T08:39:01Z

lib/dependencies/SystemPlugin.js

+
+				parser.hooks.call.for("System.import").tap("SystemPlugin", expr => {
+					if(shouldWarn) {
+						compilation.warnings.push(new SystemImportDeprecationWarning(parser.state.module, expr.loc));


Still not happy with this. To make the warning cache correctly, it has to be in module.warnings, but I will try changing it myself.

- `system: undefined`: Warns if `System.import()` is used - `system: true`: Disable warning - `system: false`: Skip `System.import()` instrumentation

webpack-bot · 2018-01-18T12:53:07Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

sokra · 2018-01-18T19:06:34Z

Thanks

webpack-bot added PR: unreviewed PR: next labels Jan 15, 2018

ooflorent commented Jan 15, 2018

View reviewed changes

webpack-bot added the PR: CI-ok label Jan 15, 2018

TheLarkInn approved these changes Jan 16, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 16, 2018

TheLarkInn requested a review from sokra January 16, 2018 04:52

TheLarkInn added PR: unreviewed and removed PR: reviewed-approved labels Jan 16, 2018

ooflorent force-pushed the disable_system_import branch from b615143 to 56aac91 Compare January 16, 2018 08:52

webpack-bot added PR: review-outdated and removed PR: unreviewed labels Jan 16, 2018

ooflorent commented Jan 16, 2018

View reviewed changes

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Jan 16, 2018

ooflorent force-pushed the disable_system_import branch from 56aac91 to 218d0a2 Compare January 16, 2018 09:19

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jan 16, 2018

sokra requested changes Jan 16, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Jan 16, 2018

ooflorent force-pushed the disable_system_import branch from 218d0a2 to d170088 Compare January 16, 2018 16:26

webpack-bot removed the PR: reviewed-changes-requested label Jan 16, 2018

ooflorent force-pushed the disable_system_import branch from d170088 to af8acab Compare January 16, 2018 16:42

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Jan 16, 2018

filipesilva mentioned this pull request Jan 16, 2018

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

Closed

ooflorent force-pushed the disable_system_import branch from af8acab to 4a59e12 Compare January 16, 2018 18:57

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jan 17, 2018

sokra requested changes Jan 18, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: review-outdated PR: CI-not-ok and removed PR: review-outdated PR: reviewed-changes-requested PR: CI-ok labels Jan 18, 2018

ooflorent closed this Jan 18, 2018

ooflorent reopened this Jan 18, 2018

Deprecate System.import() parser plugin

3fb63f9

- `system: undefined`: Warns if `System.import()` is used - `system: true`: Disable warning - `system: false`: Skip `System.import()` instrumentation

ooflorent force-pushed the disable_system_import branch from b4529b2 to 3fb63f9 Compare January 18, 2018 10:34

sokra closed this Jan 18, 2018

sokra reopened this Jan 18, 2018

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jan 18, 2018

sokra approved these changes Jan 18, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jan 18, 2018

sokra merged commit 41edb20 into webpack:next Jan 18, 2018

filipesilva mentioned this pull request Jan 22, 2018

fix(@angular/cli): use webpack 4 angular/angular-cli#8611

Closed

14 tasks

filipesilva mentioned this pull request May 4, 2018

fix(aio): remove use of System.import from PrettyPrinter angular/angular#23700

Closed

Uh oh!

Disable System.import parser plugin by default #6321

Disable System.import parser plugin by default #6321

Uh oh!

Conversation

ooflorent commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheLarkInn commented Jan 16, 2018

Uh oh!

sokra commented Jan 16, 2018

Uh oh!

ooflorent commented Jan 16, 2018

Uh oh!

ooflorent commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipesilva commented Jan 16, 2018

Uh oh!

ooflorent commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented Jan 18, 2018

Uh oh!

sokra commented Jan 18, 2018

Uh oh!

Uh oh!

ooflorent commented Jan 15, 2018 •

edited

Loading