angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
96.19k stars 25.46k forks source link

Unification of 'imports', 'declares' and 'providers' using a single attribute (e.g.: 'imports') in NgModule #10746

Closed rogerpadilla closed 8 years ago

rogerpadilla commented 8 years ago

I'm submitting a ... (check one with "x")

[ ] bug report => search github for a similar issue or PR before submitting
[*] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior The developer needs to think on three different options (imports, declares or providers) for deciding where to put the related code.

@NgModule({
  imports:      [ CommonModule, FormsModule ],
  declarations: [ ContactComponent, HighlightDirective, AwesomePipe ],
  exports:      [ ContactComponent ],
  providers:    [ ContactService ]
})
export class ContactModule { }

Expected/desired behavior Use a single attribute (e.g.: 'imports') to simplify the way things should be imported by a module. The framework should be smart enough to detect (at runtime) the type of each imported entry (if required).

Reproduction of the problem If the current behavior is a bug or you can illustrate your feature request better with an example, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (you can use this template as a starting point: http://plnkr.co/edit/tpl:AvJOMERrnz94ekVua0u5).

What is the expected behavior?

@NgModule({
  imports:      [ CommonModule, FormsModule,                     
                  ContactComponent, HighlightDirective, AwesomePipe,
                  ContactService]
  exports:      [ ContactComponent ]
})
export class ContactModule { }

What is the motivation / use case for changing the behavior? Simplify. Less code to think about, less code to write, and less code to maintain.

Please tell us about your environment:

marcalj commented 8 years ago

Please consider at least warn in console:

I've spent some hours with this dumb error with a hard-to-recognise error, also coding at late night doesn't help :P

kylecordes commented 8 years ago

By the way, this item brings up an annoying limitation/irritation. The TypeScript type system is quite good; it can express many kinds of types reasonably well. Unfortunately, it cannot yet express types related to the presence of certain decorators. But many (most?) of the most commonly used pseudo-types in Angular 2, for example the notion of a component, a directive, a pipe, a module, and so on, are all typed "any" in TypeScript. Therefore we don't get any help from TypeScript for the problem that this feature request brings up. All we get is a runtime (or eventually, template pre-compiler time) error because we tried to pass in the wrong kind of thing.

So as long as we are asking for ponies, which we almost certainly will not get because the Angular team has expressed a desire to not make any more breaking API changes, here's the pony I want. I would like to somehow have the essential and commonly used pseudo-types in Angular somehow become real types instead so that we get comprehensive IDE and compiler assistance for topics like the imports versus declarations versus providers from this feature request.

bharadwaj509 commented 8 years ago

I would also like to add to this. If we have two components other than the main app component and if these two components are sharing a single service two communicate, I feel, it would be good if we could mention it some in the app.module.ts.

For reference here is the link of the sample service sharing between child components. link

Here in the code, both the child components are sharing the service. But I would not about it until I look into the code.

If there is a way we could place them in the app.module.ts file it would be great.

aluanhaddad commented 8 years ago

@kylecordes You bring up a very real problem, there are a bunch of issues open in https://github.com/Microsoft/TypeScript to enhance type inference based on the presence of decorators in all sorts of contexts that will be beneficial to tooling and static error detection.

There are however, some related issues aspects that are worth considering.

The XXXX_PROVIDERS pattern that Angular exposes and consumes has some fundamental problems.

It is opaque: While it is true that the TypeScript language does not, yet, capture the type of a class as modified by an applied decorator, the fidelity of type information available via providers is far lower than it has to be even now. For example, if someone inserts a Pipe into a provider array, you cannot even see the name of the type at compile time. If tuple types were used instead of array types, this only requires a very simple, specialized generic identity function, you could at least see statically what kind of things you are registering.

export function tupled<
    T1 extends new (...args) => T1I, T1I, 
    T2 extends new (...args) => T2I, T2I
    >(a: T1, b: T2): [T1, T2];

export function tupled<
    T1 extends new (...args) => T1I, T1I, 
    T2 extends new (...args) => T2I, T2I, 
    T3 extends new (...args) => T3I, T3I
    >(a: T1, b: T2, c: T3): [T1, T2, T3];

export function tupled<
    T1 extends new (...args) => T1I, T1I,
    T2 extends new (...args) => T2I, T2I,
    T3 extends new (...args) => T3I, T3I,
    T4 extends new (...args) => T4I, T4I
    >(a: T1, b: T2, c: T3, d: T4): [T1, T2, T3, T4];

export function tupled<
    T1 extends new (...args) => T1I, T1I,
    T2 extends new (...args) => T2I, T2I,
    T3 extends new (...args) => T3I, T3I,
    T4 extends new (...args) => T4I, T4I,
    T5 extends new (...args) => T5I, T5I
    >(a: T1, b: T2, c: T3, d: T4, e: T5): [T1, T2, T3, T4, T5];

export function tupled<
    T1 extends new (...args) => T1I, T1I,
    T2 extends new (...args) => T2I, T2I,
    T3 extends new (...args) => T3I, T3I,
    T4 extends new (...args) => T4I, T4I,
    T5 extends new (...args) => T5I, T5I,
    T6 extends new (...args) => T6I, T6I
    >(a: T1, b: T2, c: T3, d: T4, e: T5, f: T6): [T1, T2, T3, T4, T5, T6];

export function tupled<
    T1 extends new (...args) => T1I, T1I,
    T2 extends new (...args) => T2I, T2I,
    T3 extends new (...args) => T3I, T3I,
    T4 extends new (...args) => T4I, T4I,
    T5 extends new (...args) => T5I, T5I,
    T6 extends new (...args) => T6I, T6I
    >(a: T1, b: T2, c?: T3, d?: T4, e?: T5, f?: T6) {
    switch (arguments.length) {
        case 2: return [a, b];
        case 3: return [a, b, c];
        case 4: return [a, b, c, d];
        case 5: return [a, b, c, d, e];
        case 6: return [a, b, c, d, e, f];
    }
}

Now you can see what is being provided, what you can use and how to replace it. Obviously this is not sufficient to tackle the problem you point out. I am only using it to illustrate that Angular does not take sufficient advantage of what TypeScript already offers to support improved tooling.

More broadly, the whole provider pattern is clumsy. It encourages conflation of compile time types and runtime values, it promotes non-idiomatic use of the language, I have seen developers adopting this pattern in their own app layers, export const XXXX_PROVIDERS : Type[], which is just awful. (notice this Type type and how fundamentally it may confuse new TypeScript programmers).

rogerpadilla commented 8 years ago

One important note, this would not (immediately) require breaking the current API, since properties different than 'imports' could be deprecated for maintaining backwards compatibility.

@bharadwaj509 you can move the 'ChildService" to the 'providers' attribute of the AppModule, and remove the "providers" attribute from 'Child1Component'; in that way you'd be able to share the same instance across the different components or sub-modules of the app (see). On the other hand, for this case, it looks like you should be using @Input and @Output of components instead of a shared service.

bharadwaj509 commented 8 years ago

@rogerpadilla yeah at the beginning I tied using @Input and @Output. But I was using router-components to display the child components. But also I wanted to use the values which are updated in the child components in the parent component. So I thought using a shared service is a good design.

bharadwaj509 commented 8 years ago

@rogerpadilla If I place the service in the "providers" attribute, all the components will be able to access it right. I just want some components to access it. Not all of them.

bharadwaj509 commented 8 years ago

Ok. I got my answer here.

Application-scoped Providers

The ContactService provider is application-scoped because Angular registers a module's providers with the application's root injector.

Architecturally, the ContactService belongs to the Contact business domain. Classes in other domains don't need the ContactService and shouldn't inject it.

We might expect Angular to offer a module-scoping mechanism to enforce this design. It doesn't. Angular module instances, unlike components, do not have their own injectors so they can't have their own provider scopes.

This omission is intentional. Angular modules are designed primarily to extend an application, to enrich the entire app with the module's capabilities.

Service scoping is rarely a problem in practice. Non-contact components can't inject the ContactService by accident. To inject ContactService, you must first import its type. Only Contact components should import the ContactService type.

rogerpadilla commented 8 years ago

"We might expect Angular to offer a module-scoping mechanism to enforce this design. It doesn't. Angular module instances, unlike components, do not have their own injectors so they can't have their own provider scopes."

@bharadwaj509 that was before rc5, now with @NgModule you can define injectors by modules, thus the providers will be limited to that domain. I. e. Just create an additional module for the contact package, and declare there Contact Service as a provider. Please let's discuss this in a separate thread.

about-code commented 8 years ago

@rogerpadilla I think your proposal of putting ContactComponent, HighlightDirective, AwesomePipe, ContactService into imports is flawed - at least if we talk about ContactModule example from the angular.io docs.

In this example ContactModule is the module which declares the things you want to import. But a module shouldn't have to import things which it declares itself. From a module perspective it doesn't make much sense to me.

about-code commented 8 years ago

Counter Proposal (Withdrawn - see edit note)

I agree with @rogerpadilla that sometimes, there seems to be quite a bit boilerplate with NgModules. For example, there are modules whose declarations[] overlap signifantly with exports[].

For example, in order for the ContactModule to consume its own Directives and Pipes it must put them into declarations. Now - as a deviation from the angular.io example - I want importers of the module being able to consume HighlightDirective and AwesomePipe, too. Then I must put them into exports as well:

@NgModule({
  imports:      [ CommonModule, FormsModule ],                     
  declarations: [ ContactComponent, HighlightDirective, AwesomePipe ],
  exports:      [ ContactComponent, HighlightDirective, AwesomePipe ],
  providers:    [ ContactService ]
})
Expected API (Proposal): Declarations being Union(declarations, exports)

I think a module which exports something should also be considered the one who declares the things it exports. Then semantically declarations could be defined as, being the union of declarations and exports and the configuration above could be reduced to:

@NgModule({
  imports:       [ CommonModule, FormsModule ],                     
  exports:       [ ContactComponent, HighlightDirective, AwesomePipe ],
  providers:     [ ContactService ]
})

These semantics should even be backward-compatible, because it wouldn't hurt, having HighlightDirective or AwesomePipe in both arrays.

Edit: the FAQ on the angular.io NgModule Docs lists re-exports as a use case. Then the premise, that a module only exports what it declares no longer holds true and I must withdraw my proposal.

pkozlowski-opensource commented 8 years ago

Duplicate of #10631

aluanhaddad commented 8 years ago

This is not a duplicate. It is a related but broader issue.

angular-automatic-lock-bot[bot] commented 5 years ago

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.