Skip to content
This repository has been archived by the owner on Nov 22, 2024. It is now read-only.

feat(ng-module-map-ngfactory-loader): introduce module-map-ngfactory loader #754

Merged
merged 2 commits into from
Jul 28, 2017
Merged

feat(ng-module-map-ngfactory-loader): introduce module-map-ngfactory loader #754

merged 2 commits into from
Jul 28, 2017

Conversation

FrozenPandaz
Copy link
Contributor

  • Please check if the PR fulfills these requirements
- [x] The commit message follows our guidelines: https://github.com/angular/universal/blob/master/CONTRIBUTING.md#commit-message-format
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
  • What modules are related to this pull-request
- [ ] aspnetcore-engine
- [ ] express-engine
- [ ] hapi-engine
- [x] module-map-ngfactory-loader
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature

  • What is the current behavior? (You can also link to an open issue here)

Currently, there is no such ngfactory loader

  • What is the new behavior (if this is a feature change)?

This introduces a ngfactory loader mainly used with the cli but basically loads modules based on a map

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

  • Other information:

alxhub
alxhub reviewed Jul 19, 2017
View reviewed changes
modules/ng-module-map-ngfactory-loader/README.md Outdated

```ts
@NgModule({
imports: [ModuleMapLoaderModule],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simplify this to:

imports: [ModuleMapLoaderModule.withMap(...)]

alxhub
alxhub reviewed Jul 19, 2017
View reviewed changes
modules/ng-module-map-ngfactory-loader/README.md Outdated
`@angular/cli` will generate this map in its main output bundle.

```ts
const { AppModuleNgFactory, moduleMap } = require('main.bundle.js');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be LAZY_MODULE_MAP per the other review?

@FrozenPandaz FrozenPandaz force-pushed the moduleloader branch 3 times, most recently from ce8ae6f to 51d07af Compare July 25, 2017 01:12
alxhub
alxhub reviewed Jul 25, 2017
View reviewed changes
modules/ng-module-map-ngfactory-loader/README.md Outdated

```ts
@NgModule({
imports: [ModuleMapLoaderModule.withMap(LAZY_MODULE_MAP]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct, I don't think. We can't know the LAZY_MODULE_MAP symbol ahead of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for like... regular usage.. if someone wants to i guess they could technically just build the map themselves. To be honest, the usage with CLI should come first... because thats the way most people will use it.. but at the same time we should describe how to use it without any other context?

Copy link
Member

@alxhub alxhub Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you missed my point - this code would being in the application, which means it's part of the bundle, which means it can't self-reference the not-yet-generated LAZY_MODULE_MAP symbol. We should get rid of the .withMap bit.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There also looks to be a typo there, the ( is unclosed. It should read ...withMap(LAZY_MODULE_MAP)]

@vikerman vikerman merged commit 6874a1a into angular:master Jul 28, 2017
@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 Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub approved these changes

@zzmp zzmp zzmp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FrozenPandaz @alxhub @zzmp @vikerman