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

0.9.1 tslint lazy load modules check not working (when nested) #352

Closed
ph55 opened this issue Mar 22, 2018 · 6 comments
Closed

0.9.1 tslint lazy load modules check not working (when nested) #352

ph55 opened this issue Mar 22, 2018 · 6 comments
Labels

Comments

@ph55
Copy link

ph55 commented Mar 22, 2018

Description:

  • Add lazy-loaded module to app routing.
    image

  • Import same module into another without lazy-load.
    image

No tslint warning thrown.

Repo:

https://github.com/ph55/nx-lazy-load

Info:

Angular CLI: 1.7.1
Node: 8.9.4
OS: win32 x64
Angular: 5.2.6
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

@angular/cli: 1.7.1
@angular-devkit/build-optimizer: 0.3.2
@angular-devkit/core: 0.3.2
@angular-devkit/schematics: 0.3.2
@ngtools/json-schema: 1.2.0
@ngtools/webpack: 1.10.1
@schematics/angular: 0.3.2
@schematics/package-update: 0.3.2
typescript: 2.6.2
webpack: 3.11.0
@ph55 ph55 changed the title 0.9.1 tslint lazy load modules check not working 0.9.1 tslint lazy load modules check not working (probably windows only) Mar 22, 2018
@skydever
Copy link
Contributor

hi @ph55

In my opinion this is the expected behaviour - the same lib can be included lazy or eager (tried to explain that here), both is possible - but if you import it lazy, you are not allowed to also import it eager within the same app/lib.

Try this with your example:
Add import { LazyAModule } from '@nx-test/lazy-a'; somewhere in your app where you did the lazy loading for lazy-a - then you should get the lint error.
(e.g. add it in apps/test/src/app/app.component.ts)

About windows:
Is the nx workspace the main directory of your project? If not, does #341 help (I referenced the modified JS file to be replace manually in node_modules, for some quick tests)?

@ph55
Copy link
Author

ph55 commented Mar 22, 2018

hi @skydever

Thanks for explanation.
You're right - if I 'eager-loading' module in same place where I 'lazy-loading' it - it throws error.
So it works.
image




But take a look at LazyCModule it's 'eager-loaded' in AppModule

image


And then both LazyAModule and LazyBModule eager loaded again when they already 'lazy-loaded' within AppModule.
Shouldn't it raise error as well ?
Or should it be handled by depConstraints.


image

AppModule
   -> LazyCModule (eager) -> LazyAModule(eager), LazyAModule (eager)
   -> LazyAModule(lazy), LazyAModule (lazy)

@ph55 ph55 changed the title 0.9.1 tslint lazy load modules check not working (probably windows only) 0.9.1 tslint lazy load modules check not working (when nested) Mar 22, 2018
@skydever
Copy link
Contributor

I am not sure what you are expecting, my thoughts on that are:

the rule does not check if a lazy loaded module is importing a module that was already eager loaded in the app or the other way round, and I think that's ok like this. lets say you have a classic shared module, you will import that module nearly everywhere, and you need to to have access to its components, also in the lazy loaded module. you don't have access to the components of LazyAModule within LazyCModule if you do not import it explicitly ...

about circular dependencies on that matter, I think this is ok too:
app -> LazyCModule (eager), LazyAModule (lazy), LazyBModule (lazy)
LazyCModule -> LazyAModule (eager), LazyBModule (eager)
LazyAModule -> no deps
LazyBModule -> no deps

@ph55
Copy link
Author

ph55 commented Mar 22, 2018

Got it. Thanks.

@ph55 ph55 closed this as completed Mar 22, 2018
@skydever
Copy link
Contributor

I am happy if I could help. Still figuring out some things myself.

I thought about the old behavior, that forbid the import of libs when they were marked lazy. You can do it with depConstraints, but in that case you have to think the other way round - you can only define what is allowed, and not what is forbidden. Maybe an additional attribute next to onlyDependOnLibsWithTags is helpful in some situations (not sure, even often?), like notDependOnLibsWithTags. At the moment I only can think of tagging all libs with eager or lazy and then define { "sourceTag": "*", "onlyDependOnLibsWithTags": ["eager"] } to get the same results...

@github-actions
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants