feat(router): auto-unwrap default exports when lazy loading #47586

alxhub · 2022-09-29T21:25:18Z

When using loadChildren or loadComponent, a common pattern is to pass a function that returns a Promise from a dynamic import:

{
  path: 'lazy',
  loadComponent: () => import('./lazy-file').then(m => m.LazyCmp),
}

The .then part of the expression selects the particular exported component symbol from the dynamically imported ES module.

ES modules can have a "default export", created with the export default modifier:

@Component({...})
export default class LazyCmp { ... }

This default export is made available to dynamic imports under the well- known key of 'default', per the ES module spec:
https://tc39.es/ecma262/#table-export-forms-mapping-to-exportentry-records

This commit adds a feature to the router to automatically dereference such default exports. With this logic, when export default is used, a .then operation to select the particular exported symbol is no longer required:

{
  path: 'lazy',
  loadComponent: () => import('./lazy-file'),
}

The above loadComponent operation will automatically use the default export of the lazy-file ES module.

This functionality works for loadChildren as well.

atscott

reviewed-for: public-api

AndrewKushnir

Reviewed-for: public-api

(just one comment about the DX for error cases)

AndrewKushnir · 2022-09-30T22:11:28Z

packages/router/test/standalone.spec.ts

+      expect(root.nativeElement.innerHTML).toContain('default exported');
+    });
+
+    it('should work for loadChildren', async () => {


I'm wondering what the DX would look like in case of missing default export? Can we add a test to check that?

I was thinking of a couple scenarios:

no exports at all (we can have an error message explaining this)

no default exports, but other symbols are exported (we can also throw a message explaining that)

WDYT?

(we can also do that in a followup PR if needed, it's not a blocker)

TypeScript will fail in both cases:

If there are no exports, the TypeScript compiler will error with File 'the/imported/thing' is not a module.

If there is no default export, it would fail the DefaultExport<T> type.

pkozlowski-opensource

LGTM

Reviewed-for: public-api
Reviewed-for: fw-core

AndrewKushnir · 2022-10-03T17:16:03Z

FYI, it looks like this change would require an update to the local patches in g3, adding the "blocked" label for now...

alxhub · 2022-10-04T16:19:25Z

Caretaker: a g3 patch (cl/478602165) is required

atscott · 2022-10-04T16:28:34Z

goldens/public-api/router/index.md

@@ -357,7 +357,7 @@ export interface IsActiveMatchOptions {
 }

 // @public
-export type LoadChildren = LoadChildrenCallback;
+export type LoadChildren = LoadChildrenCallback | ɵDeprecatedLoadChildren;


ɵDeprecatedLoadChildren now appears in the public API even though it's not exported and is unused externally. What about moving the LoadChildren type to its own file that we patch in g3 to add ɵDeprecatedLoadChildren?

Yep - that's fine. The goal is to not need to touch the definition of LoadChildren at all in the patch file - that's what leads to brittle patch files.

packages/router/src/deprecated_load_children.ts

atscott · 2022-10-04T16:30:20Z

packages/router/src/router_config_loader.ts

@@ -122,6 +123,9 @@ export class RouterConfigLoader {

  private loadModuleFactoryOrRoutes(loadChildren: LoadChildren):
      Observable<NgModuleFactory<any>|Routes> {
+    if (typeof loadChildren === 'string') {
+      return deprecatedLoadChildrenString(this.injector, loadChildren);


What if we unconditionally call deprecatedLoadChildrenString from here and make it an empty function externally? We at least have a single file we can patch in g3 then.

This doesn't work, because the typeof needs to be performed within loadModuleFactoryOrRoutes to narrow the type of loadChildren for the rest of the function.

Okay, this can work with a cast, so I applied that change.

This commit breaks out the code in @angular/router needed to support the deprecated `loadChildren` signature into a separate file, which reduces the opportunity for conflicts when patching that behavior in g3.

When using `loadChildren` or `loadComponent`, a common pattern is to pass a function that returns a `Promise` from a dynamic import: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file').then(m => m.LazyCmp), } ``` The `.then` part of the expression selects the particular exported component symbol from the dynamically imported ES module. ES modules can have a "default export", created with the `export default` modifier: ```typescript @component({...}) export default class LazyCmp { ... } ``` This default export is made available to dynamic imports under the well- known key of `'default'`, per the ES module spec: https://tc39.es/ecma262/#table-export-forms-mapping-to-exportentry-records This commit adds a feature to the router to automatically dereference such default exports. With this logic, when `export default` is used, a `.then` operation to select the particular exported symbol is no longer required: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file'), } ``` The above `loadComponent` operation will automatically use the `default` export of the `lazy-file` ES module. This functionality works for `loadChildren` as well.

ngbot · 2022-10-04T19:02:31Z

I see that you just added the action: merge label, but the following checks are still failing:
     status "ci/angular: size" is failing
     status "ci/circleci: test" is failing
     status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir · 2022-10-04T21:41:00Z

This PR was merged into the repository by commit da58801.

When using `loadChildren` or `loadComponent`, a common pattern is to pass a function that returns a `Promise` from a dynamic import: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file').then(m => m.LazyCmp), } ``` The `.then` part of the expression selects the particular exported component symbol from the dynamically imported ES module. ES modules can have a "default export", created with the `export default` modifier: ```typescript @component({...}) export default class LazyCmp { ... } ``` This default export is made available to dynamic imports under the well- known key of `'default'`, per the ES module spec: https://tc39.es/ecma262/#table-export-forms-mapping-to-exportentry-records This commit adds a feature to the router to automatically dereference such default exports. With this logic, when `export default` is used, a `.then` operation to select the particular exported symbol is no longer required: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file'), } ``` The above `loadComponent` operation will automatically use the `default` export of the `lazy-file` ES module. This functionality works for `loadChildren` as well. PR Close #47586

…47586) When using `loadChildren` or `loadComponent`, a common pattern is to pass a function that returns a `Promise` from a dynamic import: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file').then(m => m.LazyCmp), } ``` The `.then` part of the expression selects the particular exported component symbol from the dynamically imported ES module. ES modules can have a "default export", created with the `export default` modifier: ```typescript @component({...}) export default class LazyCmp { ... } ``` This default export is made available to dynamic imports under the well- known key of `'default'`, per the ES module spec: https://tc39.es/ecma262/#table-export-forms-mapping-to-exportentry-records This commit adds a feature to the router to automatically dereference such default exports. With this logic, when `export default` is used, a `.then` operation to select the particular exported symbol is no longer required: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file'), } ``` The above `loadComponent` operation will automatically use the `default` export of the `lazy-file` ES module. This functionality works for `loadChildren` as well. PR Close angular#47586

This commit breaks out the code in @angular/router needed to support the deprecated `loadChildren` signature into a separate file, which reduces the opportunity for conflicts when patching that behavior in g3. PR Close angular#47586

…47586) When using `loadChildren` or `loadComponent`, a common pattern is to pass a function that returns a `Promise` from a dynamic import: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file').then(m => m.LazyCmp), } ``` The `.then` part of the expression selects the particular exported component symbol from the dynamically imported ES module. ES modules can have a "default export", created with the `export default` modifier: ```typescript @component({...}) export default class LazyCmp { ... } ``` This default export is made available to dynamic imports under the well- known key of `'default'`, per the ES module spec: https://tc39.es/ecma262/#table-export-forms-mapping-to-exportentry-records This commit adds a feature to the router to automatically dereference such default exports. With this logic, when `export default` is used, a `.then` operation to select the particular exported symbol is no longer required: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file'), } ``` The above `loadComponent` operation will automatically use the `default` export of the `lazy-file` ES module. This functionality works for `loadChildren` as well. PR Close angular#47586

This commit breaks out the code in @angular/router needed to support the deprecated `loadChildren` signature into a separate file, which reduces the opportunity for conflicts when patching that behavior in g3. PR Close angular#47586

…47586) When using `loadChildren` or `loadComponent`, a common pattern is to pass a function that returns a `Promise` from a dynamic import: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file').then(m => m.LazyCmp), } ``` The `.then` part of the expression selects the particular exported component symbol from the dynamically imported ES module. ES modules can have a "default export", created with the `export default` modifier: ```typescript @component({...}) export default class LazyCmp { ... } ``` This default export is made available to dynamic imports under the well- known key of `'default'`, per the ES module spec: https://tc39.es/ecma262/#table-export-forms-mapping-to-exportentry-records This commit adds a feature to the router to automatically dereference such default exports. With this logic, when `export default` is used, a `.then` operation to select the particular exported symbol is no longer required: ```typescript { path: 'lazy', loadComponent: () => import('./lazy-file'), } ``` The above `loadComponent` operation will automatically use the `default` export of the `lazy-file` ES module. This functionality works for `loadChildren` as well. PR Close angular#47586

angular-automatic-lock-bot · 2022-11-04T16:07:02Z

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

alxhub requested a review from atscott September 29, 2022 21:25

alxhub added area: router target: major This PR is targeted for the next major release labels Sep 29, 2022

ngbot bot added this to the Backlog milestone Sep 29, 2022

alxhub modified the milestones: Backlog, v15 feature freeze blockers Sep 29, 2022

alxhub force-pushed the v15/router/default branch 3 times, most recently from 6444f44 to 2b5209b Compare September 29, 2022 23:51

angular-robot bot added the feature Issue that requests a new feature label Sep 29, 2022

atscott approved these changes Sep 30, 2022

View reviewed changes

pullapprove bot requested review from atscott, jessicajaniuk and pkozlowski-opensource September 30, 2022 15:09

atscott approved these changes Sep 30, 2022

View reviewed changes

pullapprove bot requested a review from dylhunn September 30, 2022 15:11

AndrewKushnir approved these changes Sep 30, 2022

View reviewed changes

pkozlowski-opensource approved these changes Oct 1, 2022

View reviewed changes

pkozlowski-opensource removed request for dylhunn and jessicajaniuk October 1, 2022 07:26

pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Oct 1, 2022

AndrewKushnir added state: blocked action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Oct 3, 2022

alxhub force-pushed the v15/router/default branch from 2b5209b to 32ddfb9 Compare October 3, 2022 21:00

alxhub added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed state: blocked labels Oct 4, 2022

atscott reviewed Oct 4, 2022

View reviewed changes

atscott requested changes Oct 4, 2022

View reviewed changes

alxhub added 2 commits October 4, 2022 11:04

refactor(router): organize deprecated loadChildren code

db63d4d

This commit breaks out the code in @angular/router needed to support the deprecated `loadChildren` signature into a separate file, which reduces the opportunity for conflicts when patching that behavior in g3.

alxhub force-pushed the v15/router/default branch from 32ddfb9 to 942d6c5 Compare October 4, 2022 18:08

alxhub requested a review from atscott October 4, 2022 18:12

atscott approved these changes Oct 4, 2022

View reviewed changes

alxhub added the action: rerun CI at HEAD label Oct 4, 2022

ngbot bot removed the action: rerun CI at HEAD label Oct 4, 2022

AndrewKushnir closed this in a2a066d Oct 4, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router): auto-unwrap default exports when lazy loading #47586

feat(router): auto-unwrap default exports when lazy loading #47586

alxhub commented Sep 29, 2022

atscott left a comment

AndrewKushnir left a comment

AndrewKushnir Sep 30, 2022

atscott Sep 30, 2022

pkozlowski-opensource left a comment

AndrewKushnir commented Oct 3, 2022

alxhub commented Oct 4, 2022

atscott Oct 4, 2022

alxhub Oct 4, 2022

atscott Oct 4, 2022

alxhub Oct 4, 2022

alxhub Oct 4, 2022

ngbot bot commented Oct 4, 2022

AndrewKushnir commented Oct 4, 2022

angular-automatic-lock-bot bot commented Nov 4, 2022

feat(router): auto-unwrap default exports when lazy loading #47586

feat(router): auto-unwrap default exports when lazy loading #47586

Conversation

alxhub commented Sep 29, 2022

atscott left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Sep 30, 2022

Choose a reason for hiding this comment

atscott Sep 30, 2022

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Oct 3, 2022

alxhub commented Oct 4, 2022

atscott Oct 4, 2022

Choose a reason for hiding this comment

alxhub Oct 4, 2022

Choose a reason for hiding this comment

atscott Oct 4, 2022

Choose a reason for hiding this comment

alxhub Oct 4, 2022

Choose a reason for hiding this comment

alxhub Oct 4, 2022

Choose a reason for hiding this comment

ngbot bot commented Oct 4, 2022

AndrewKushnir commented Oct 4, 2022

angular-automatic-lock-bot bot commented Nov 4, 2022