Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register multiple component in JssModule.forChild method #83

Merged
merged 11 commits into from
Dec 12, 2018
Merged

Register multiple component in JssModule.forChild method #83

merged 11 commits into from
Dec 12, 2018

Conversation

agalbulev
Copy link
Contributor

@agalbulev agalbulev commented Nov 29, 2018

Description

Hi JSS team, I am Aleksandar and I work in Virtual Affairs. I saw that you now have a feature for lazy loading of component which is great. I try to made lazy loading module with multiple components which will be great feature for us but it not possible with current implementation. I made research in your code and found that this is possible with little change which can be make on JssModule.forChild function.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@agalbulev
Copy link
Contributor Author

Do you think that I need to improve something more? I made several test about my implementation and I think that work fine. Can some one make review of changes?

@kamsar
Copy link
Contributor

kamsar commented Dec 11, 2018

@agalbulev I did a quick test and this change does not seem to break the existing lazy sample, at least.

It's not entirely clear to me what the procedure would be to register multiple components, and that needs to be documented and tested in order to merge. To make this simpler, we've moved the docs into this repo so that they can be directly updated as part of PRs. The relevant documentation is here - please enhance it to provide examples of lazy loading multiple components. I've merged master into your PR so the docs are available on your branch.

@agalbulev
Copy link
Contributor Author

@kamsar I added the documentation and example.

Copy link
Contributor

@kamsar kamsar left a comment

Choose a reason for hiding this comment

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

I was unable to make this work using the documentation (which is well written btw!). The most basic scenario I could come up with:

  • Take the angular styleguide app
  • Alter styleguide-angular-lazy-loading.module.ts to register the existing lazy component demo using the new array syntax:
JssModule.forChild([
      { name: 'StyleguideAngularLazyLoadingComponent', type: StyleguideAngularLazyLoadingComponent },
      //{ name: 'StyleguideAngularLazyLoadingComponent2', type: StyleguideAngularLazyLoading2Component}
    ]),
  • Load up the Angular sample with jss start. I get this JS error on the styleguide route when it attempts to load the lazy module:
vendor.js:58495 ERROR Error: Uncaught (in promise): Error: JssComponentFactoryService: Lazy load module for component "StyleguideAngularLazyLoading" missing DYNAMIC_COMPONENT provider. Missing JssModule.forChild()?
Error: JssComponentFactoryService: Lazy load module for component "StyleguideAngularLazyLoading" missing DYNAMIC_COMPONENT provider. Missing JssModule.forChild()?
    at sitecore-jss-sitecore-jss-angular.ts:14
    at ZoneDelegate.push../node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (polyfills.js:3268)

I attempted to get it to load two components as well, but same error. I had rebuilt the sitecore-jss-angular library before doing this.

])
],
declarations: [
MyComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't the declarations need to be FirstComponent, SecondComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this branch you can find my test site https://github.com/agalbulev/jss/tree/feature/test_multi_lazy. Can you send to me your configuration because I don't have this problem with my test site?

Copy link
Contributor Author

@agalbulev agalbulev Dec 12, 2018

Choose a reason for hiding this comment

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

Can you also check that the component name 'StyleguideAngularLazyLoadingComponent', from your test site, is equal to the property 'path' for this component in 'app-components.module.ts'? If they aren't equal you can have this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Argh I'd pasted the name of the class into the forChild() registration complete with the Component suffix. Which the JSS component name didn't have.

Chalk another one up to Angular's totally amazing error messaging :trollface:

kamsar
kamsar previously approved these changes Dec 12, 2018
@kamsar kamsar merged commit 0a1b2fd into Sitecore:master Dec 12, 2018
@kamsar
Copy link
Contributor

kamsar commented Jan 8, 2019

@agalbulev @spike1292 ran into an issue that seems strongly correlated with this PR that it seems to have broken AOT (jss build) compilation. Reproduction is pretty easy: make a new Angular app with the CLI and run jss build. It works fine with non-AOT builds (i.e. jss start).

https://sitecore.stackexchange.com/questions/15903/deploy-jss-app-on-sitecore-instance

Do you guys see any reason why that should suddenly be required? Certainly other modules are using Module.forXXX() functions and they have no issues with AOT - and this works fine if I downgrade to [email protected] without these changes.

@agalbulev
Copy link
Contributor Author

agalbulev commented Jan 8, 2019

@kamsar Yes, today I find this problem too. I think this is a problem associated with an Angular AOT compiler. A found how we can fix the problem, without using forChild() method into a lazy loading module.

  {
      ngModule: JssModule,
      providers: [
        {
          provide: DYNAMIC_COMPONENT,
          useValue: {
            'FirstComponent': FirstComponent,
	    'SecondComponent': SecondComponent
          }
        },
        {
          provide: ANALYZE_FOR_ENTRY_COMPONENTS,
          useValue: [
		{ name: 'FirstComponent', type: FirstComponent },
      		{ name: 'SecondComponent', type: SecondComponent }
	  ]
          multi: true
        }
      ]
    }

Only we need to export DYNAMIC_COMPONENT provider.

Also, we can revert forChild() method to old simple implementation. Connected with this issue https://stackoverflow.com/questions/47686638/featuremodule-fails-during-an-aot-build-when-static-forroot-has-arguments, I think that this can fix our issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants