Support `import()` syntax for loadChildren in ViewEngine #13948

filipesilva · 2019-03-19T15:13:38Z

This feature ONLY matches the format below:

loadChildren: () => import('IMPORT_STRING').then(m => m.EXPORT_NAME)

It will not match nor alter variations, for instance:

not using arrow functions
not using m as the module argument
using await instead of then
using a default export (AoT Compiler unable to resolve module with "export default class" angular#11402)

The only parts that can change are the ones in caps: IMPORT_STRING and EXPORT_NAME.

filipesilva · 2019-03-19T15:14:27Z

Blocked on angular/angular#29392

packages/ngtools/webpack/src/transformers/import_factory.ts

alan-agius4 · 2019-03-26T14:19:54Z

packages/ngtools/webpack/src/angular_compiler_plugin.ts

@@ -103,6 +104,8 @@ export interface AngularCompilerPluginOptions {
  nameLazyFiles?: boolean;
  logger?: logging.Logger;
  directTemplateLoading?: boolean;
+  discoverLazyRoutes?: boolean;


Can you kindly explain the reasoning behind this option?

When using the loadChildren string syntax, @ngtools/webpack must query @angular/compiler-cli via a private API to know which lazy routes exist. This increases build and rebuild time.

When using Ivy, the string syntax is not supported at all. Thus we shouldn't attempt that.

This commit also exists in #13700 for slightly different reasons. There, it is meant to disable this sort of processing for compilations that do not need it.

Thanks for the clarification, it makes more sense now :) As to disable lazy route discovery under ivy we can use enableIvy, but seeing that other PR. I can now see the use case.

Should we warn (or probably error) in the transformer (or somewhere else) if the string form is found when this option is off?

Users may not know why their applications are not working.

I think not because that means we need to always check, which incurs a performance penalty on every AOT build.

The "right way" of doing that probably a Ivy migration when it's stable.

let's try to generally answer questions like this in the form of a comment in the code - no one later will find this thread

clydin · 2019-03-26T14:10:06Z

packages/ngtools/webpack/src/transformers/import_factory.ts

+            return replaceImport(node, context, emitWarning);
+          }
+
+          return ts.visitEachChild(node, visitNode, context);


If ngc always creates the variable declaration at the root level then only the source file's children should need to be analyzed. Walking the entire tree can be avoided. The regex test above can be avoided as well since top level nodes are typically fairly minimal and testing a large block of text will be much less efficient.

That is a better approach, yes. I modified the code to only recurse into VariableStatement to find VariableDeclaration within, since the structure is VariableStatement -> VariableDeclarationList -> VariableDeclaration. PTAL

clydin · 2019-03-26T14:44:01Z

packages/ngtools/webpack/src/angular_compiler_plugin.ts

@@ -103,6 +104,8 @@ export interface AngularCompilerPluginOptions {
  nameLazyFiles?: boolean;
  logger?: logging.Logger;
  directTemplateLoading?: boolean;
+  discoverLazyRoutes?: boolean;


Should we warn (or probably error) in the transformer (or somewhere else) if the string form is found when this option is off?

Users may not know why their applications are not working.

packages/ngtools/webpack/src/transformers/import_factory.ts

packages/ngtools/webpack/src/angular_compiler_plugin.ts

packages/ngtools/webpack/src/transformers/import_factory.ts

packages/ngtools/webpack/src/transformers/import_factory_spec.ts

alan-agius4 · 2019-03-28T11:46:42Z

packages/ngtools/webpack/src/transformers/import_factory.ts

+): ts.TransformerFactory<ts.SourceFile> {
+  return (context: ts.TransformationContext) => {
+    return (sourceFile: ts.SourceFile) => {
+      const warning = `


Should this be a warning or an error, since it will cause a runtime error?

It won't necessarily cause a runtime error. VE can still use both types of imports.

alan-agius4 · 2019-03-28T13:53:22Z

tests/legacy-cli/e2e/tests/build/lazy-load-syntax.ts

 import { expectToFail } from '../../utils/utils';

 export default async function () {
-  const projectName = 'ivy-lazy-loading';
+  const argv = getGlobalVariable('argv');
+  const ivyProject = argv['ivy'];


How are these run during the e2e-cli tests, Isn't the ivy arg used only in the cli snapshots?

Hence we would only be testing a subset of scenarios during the e2e's in PRs, to be more precise the VE tests.

We only have a limited tests for Ivy during E2E, and removing some might be risky. Unless we we have some e2e-cli-ivy job for PRs.

Yes, Ivy tests are only run for snapshots now. I agree with you that we would love coverage. I think moving the ivy job to not use the snapshots, and to always run, is a good approach. We're starting to have more cde that interacts with Ivy so we should have confidence in PRs for it.

alan-agius4

Other than the above questions.

LGTM

alexeagle · 2019-03-29T17:25:30Z

.circleci/config.yml

@@ -106,24 +106,17 @@ jobs:
          path: /tmp/dist
          destination: cli/new-production

-  e2e-cli-ng-snapshots:
+  e2e-cli-ivy:


nit: why switch the order of these? makes the delta harder to review

To keep similar e2e jobs (e2e-cli and e2e-cli-ivy) together.

The old e2e-cli-ng-snapshots was after e2e-cli-ivy-snapshots because they shared the snapshot flag and only ran on some builds. Now e2e-cli-ivy-snapshots is instead e2e-cli-ivy and should instead be next to e2e-cli.

alexeagle · 2019-03-29T17:26:49Z

packages/ngtools/webpack/src/angular_compiler_plugin.ts

@@ -103,6 +104,8 @@ export interface AngularCompilerPluginOptions {
  nameLazyFiles?: boolean;
  logger?: logging.Logger;
  directTemplateLoading?: boolean;
+  discoverLazyRoutes?: boolean;


let's try to generally answer questions like this in the form of a comment in the code - no one later will find this thread

alexeagle · 2019-03-29T17:28:12Z

packages/ngtools/webpack/src/transformers/import_factory.ts

+
+loadChildren: () => import('IMPORT_STRING').then(m => m.EXPORT_NAME)
+
+Please note that only IMPORT_STRING and EXPORT_NAME can be replaced in this format.`;


link to the guide again, if ppl get confused we'll be able to help them there

…iewEngine This feature ONLY matches the format below: ``` loadChildren: () => import('IMPORT_STRING').then(m => m.EXPORT_NAME) ``` It will not match nor alter variations, for instance: - not using arrow functions - not using `m` as the module argument - using `await` instead of `then` - using a default export (angular/angular#11402) The only parts that can change are the ones in caps: IMPORT_STRING and EXPORT_NAME.

…option

angular-automatic-lock-bot · 2019-09-13T00:41:10Z

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

googlebot added the cla: yes label Mar 19, 2019

filipesilva changed the title ~~Import syntax ve~~ Support import() syntax for loadChildren in ViewEngine Mar 19, 2019

filipesilva force-pushed the import-syntax-ve branch from 07b7772 to 8b10696 Compare March 19, 2019 15:24

filipesilva mentioned this pull request Mar 19, 2019

fix(ivy): don't load factories when using Ivy angular/angular#28685

Closed

14 tasks

filipesilva commented Mar 19, 2019

View reviewed changes

packages/ngtools/webpack/src/transformers/import_factory.ts Outdated Show resolved Hide resolved

filipesilva force-pushed the import-syntax-ve branch 2 times, most recently from 2a37c0c to 451d2cb Compare March 25, 2019 15:44

filipesilva mentioned this pull request Mar 25, 2019

Ivy guide update angular/angular#29506

Closed

14 tasks

filipesilva force-pushed the import-syntax-ve branch from 451d2cb to 2d7022f Compare March 26, 2019 11:44

filipesilva requested review from clydin and alan-agius4 March 26, 2019 13:08

filipesilva added the target: major This PR is targeted for the next major release label Mar 26, 2019

filipesilva marked this pull request as ready for review March 26, 2019 13:08

alan-agius4 reviewed Mar 26, 2019

View reviewed changes

clydin reviewed Mar 26, 2019

View reviewed changes

alan-agius4 reviewed Mar 26, 2019

View reviewed changes

packages/ngtools/webpack/src/angular_compiler_plugin.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Mar 26, 2019

View reviewed changes

packages/ngtools/webpack/src/transformers/import_factory.ts Show resolved Hide resolved

alan-agius4 reviewed Mar 26, 2019

View reviewed changes

packages/ngtools/webpack/src/transformers/import_factory_spec.ts Outdated Show resolved Hide resolved

filipesilva force-pushed the import-syntax-ve branch from 2d7022f to 38fff6a Compare March 27, 2019 11:15

alan-agius4 reviewed Mar 28, 2019

View reviewed changes

alan-agius4 approved these changes Mar 28, 2019

View reviewed changes

filipesilva force-pushed the import-syntax-ve branch 3 times, most recently from a4ac54e to 82f25f9 Compare March 28, 2019 17:48

alexeagle approved these changes Mar 29, 2019

View reviewed changes

filipesilva added 3 commits March 29, 2019 17:53

feat(@ngtools/webpack): add option to control lazy route discovery

edbace1

feat(@angular-devkit/build-angular): add experimentalImportFactories …

ca4cea9

…option

filipesilva added 3 commits March 29, 2019 17:53

test: test lazy-loading syntax for VE and Ivy

429fd62

refactor(@ngtools/webpack): simplify a call using await

6166c8d

ci: add ivy track for all prs

c9b3071

filipesilva force-pushed the import-syntax-ve branch from 82f25f9 to c9b3071 Compare March 29, 2019 17:55

mgechev merged commit 27a84a8 into angular:master Mar 29, 2019

filipesilva mentioned this pull request Apr 19, 2019

Add experimentalImportFactories when migrating to loadChildren import syntax #14218

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `import()` syntax for loadChildren in ViewEngine #13948

Support `import()` syntax for loadChildren in ViewEngine #13948

filipesilva commented Mar 19, 2019 •

edited

Loading

filipesilva commented Mar 19, 2019

alan-agius4 Mar 26, 2019

filipesilva Mar 26, 2019

alan-agius4 Mar 26, 2019 •

edited

Loading

clydin Mar 26, 2019

filipesilva Mar 27, 2019

alexeagle Mar 29, 2019

filipesilva Mar 29, 2019

clydin Mar 26, 2019

filipesilva Mar 27, 2019

clydin Mar 26, 2019

alan-agius4 Mar 28, 2019

filipesilva Mar 28, 2019

alan-agius4 Mar 28, 2019 •

edited

Loading

filipesilva Mar 28, 2019

alan-agius4 left a comment •

edited

Loading

alexeagle Mar 29, 2019

filipesilva Mar 29, 2019

filipesilva Mar 29, 2019

alexeagle Mar 29, 2019

alexeagle Mar 29, 2019

filipesilva Mar 29, 2019

angular-automatic-lock-bot bot commented Sep 13, 2019


		loadChildren: () => import('IMPORT_STRING').then(m => m.EXPORT_NAME)

		Please note that only IMPORT_STRING and EXPORT_NAME can be replaced in this format.`;

Support import() syntax for loadChildren in ViewEngine #13948

Support import() syntax for loadChildren in ViewEngine #13948

Conversation

filipesilva commented Mar 19, 2019 • edited Loading

filipesilva commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-agius4 Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-agius4 Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-agius4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 13, 2019

Support `import()` syntax for loadChildren in ViewEngine #13948

Support `import()` syntax for loadChildren in ViewEngine #13948

filipesilva commented Mar 19, 2019 •

edited

Loading

alan-agius4 Mar 26, 2019 •

edited

Loading

alan-agius4 Mar 28, 2019 •

edited

Loading

alan-agius4 left a comment •

edited

Loading