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

docs: add NgModule docs #20306

Closed
wants to merge 3 commits into from

Conversation

kapunahelewong
Copy link
Contributor

@kapunahelewong kapunahelewong commented Nov 9, 2017

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

NgModules info is in two docs.
Issue Number: N/A

What is the new behavior?

NgModules is broken up by topic.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Closes #21257 #18957 #18799 #17825

@kapunahelewong kapunahelewong changed the title docs(aio): add NgModule docs [WIP]docs(aio): add NgModule docs Nov 9, 2017
@gkalpak gkalpak closed this Nov 17, 2017
@gkalpak gkalpak reopened this Nov 17, 2017

* **[_providers_](#providers)** — there are none to start but you are likely to add some soon.
You must declare every component in an `NgModule` class.
Copy link
Contributor

Choose a reason for hiding this comment

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

...every component in exactly one NgModule...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* **[_providers_](#providers)** — there are none to start but you are likely to add some soon.
You must declare every component in an `NgModule` class.
If you use a component without declaring it, Angular returns an
error message in the browser console.
Copy link
Contributor

Choose a reason for hiding this comment

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

is on the browser console necessary? Where else would the error be? Less words is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


```

The `import` statements at the top make the code in those files available to
Copy link
Contributor

Choose a reason for hiding this comment

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

import is standard ES6 stuff, i don't think we need to talk about it. We just expect the reader to know. Less is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.


The `@NgModule` properties for the minimal `AppModule` generated by the CLI are as follows:
* **_declarations_**—this application's lone component.
* **_imports_**—the `BrowserModule` that applications need to run in a browser.
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what you are trying to say here. I would say: Import BrowserModule to have browser specific services such as DOM rendering, sanitization, and location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


Whereas the `imports` array is for importing NgModules,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we talking about imports all of a sudden? Also the grammar of the sentence is weird. I would omit the import as it it not relevant to declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


### The _declarations_ array
**Only `@NgModule` classes** go in the `imports` array. Do not put any other kind of class in `imports`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say only NgModule references go in .... (The fact that they are classes is incidental). Also is the second sentence necessary? it says the same as first. Less is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

You'll learn to create two other kinds of classes —
[directives](guide/attribute-directives) and [pipes](guide/pipes) —
that you must also add to the `declarations` array.
A common use of the `declarations` array is using directives.
Copy link
Contributor

Choose a reason for hiding this comment

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

more active voice: Use declarations for ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Done.

[directives](guide/attribute-directives) and [pipes](guide/pipes) —
that you must also add to the `declarations` array.
A common use of the `declarations` array is using directives.
For example, when you create a directive and use it in a template, such as in the [Attribute Directives](guide/attribute-directives) page, you need to add it to the relevant NgModule `declarations` array. Relevant here means the NgModule that the directive or component belongs to. In simple apps with only the root module, everything belongs to that one module. However, for apps with more than one module, certain directives, components, and pipes belong to one module or another. To use a directive, component, or pipe in a module, you must do a few things:
Copy link
Contributor

Choose a reason for hiding this comment

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

this sounds like a repeat what you already told me above. Also why "Attribute Directives"? Why not directives in general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Fixed.


</div>
Those three steps look like the following. In the file where you create your directive, export it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there is way to much text/detail about how to do this. Assume reader knows how to import/export things in ES6, it should be prerequisite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenniferfell I agree with this point (and am adding pre-requisites at the top), but as this kind of guidance is throughout the docs and this is intro level doc, I'd like to get your thoughts on how we can apply this systematically yet make sure there's a clear point of reference for those who don't know.

Before DI can inject a service, it must create that service with the help of a _provider_.
You can tell DI about a service's _provider_ in a number of ways.
Among the most popular ways is to register the service in the root `ngModule.providers` array, which will make that service available _everywhere_.
The module's `imports` array appears exclusively in the `@NgModule` metadata object.
Copy link
Contributor

Choose a reason for hiding this comment

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

don't refer to other @NgModule as classes but references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks!

@gkalpak gkalpak closed this Nov 28, 2017
@gkalpak gkalpak reopened this Nov 28, 2017
@jenniferfell jenniferfell added this to the 5.1.2-sprint milestone Dec 8, 2017
@jenniferfell jenniferfell self-requested a review December 8, 2017 22:14
@kapunahelewong kapunahelewong force-pushed the wong-ngmodules branch 4 times, most recently from cec5d62 to 01d5931 Compare December 9, 2017 19:13
@mary-poppins
Copy link

You can preview 01d5931 at https://pr20306-01d5931.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 43af2a4 at https://pr20306-43af2a4.ngbuilds.io/.

@mary-poppins
Copy link

You can preview f045256 at https://pr20306-f045256.ngbuilds.io/.

@mary-poppins
Copy link

You can preview dc7b19c at https://pr20306-dc7b19c.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 788bcb5 at https://pr20306-788bcb5.ngbuilds.io/.

@alexeagle
Copy link
Contributor

@kara 💚 no google3 change

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

From a quick look, there seem to be some changes that should not be included and Travis is red. I know the current failures are unrelated to this PR, but they must have been fixed on master and might have been covering real failures.

I will take more thorough look tomorrow (pre- or post-merge 😃).

BTW, not directly related to this PR, but the unit test files for examples are not actually used in any tests (afaict). This means they can become outdated ot even break without us noticing, which might be frustrating for people trying to use them (e.g. by downloading the zips) 😞


checkAnchorLinksProcessor.$enabled = true;
checkAnchorLinksProcessor.$enabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

I assume these are not supposed to be part of the PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh man, yeah! I forgot I'd changed this while I wrote so I could see it rendered. Thanks for catching this.

aio/yarn.lock Outdated
resolved "https://registry.yarnpkg.com/debug/-/debug-2.6.8.tgz#e731531ca2ede27d188222427da17821d68ff4fc"
dependencies:
ms "2.0.0"

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change should be included in the PR either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this. I've fixed it. :)

@gkalpak
Copy link
Member

gkalpak commented Jan 10, 2018

The Travis failures are "legit" 😃

@mary-poppins
Copy link

You can preview a8ee8f7 at https://pr20306-a8ee8f7.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9871e8e at https://pr20306-9871e8e.ngbuilds.io/.

@alexeagle alexeagle removed the action: merge The PR is ready for merge by the caretaker label Jan 10, 2018
@alexeagle
Copy link
Contributor

Looks not ready to merge, due to Travis failure:


ERROR in Error: Could not resolve module ./app/customers/customers.module relative to /home/travis/build/angular/angular/aio/content/examples/lazy-loading/src/app/app-routing.module.ts
    at StaticSymbolResolver.getSymbolByModule (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler/bundles/compiler.umd.js:29757:30)
    at StaticReflector.resolveExternalReference (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler/bundles/compiler.umd.js:31594:62)
    at parseLazyRoute (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler/bundles/compiler.umd.js:29043:55)
    at listLazyRoutes (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler/bundles/compiler.umd.js:29005:36)
    at visitLazyRoute (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler/bundles/compiler.umd.js:31167:47)
    at AotCompiler.listLazyRoutes (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler/bundles/compiler.umd.js:31135:20)
    at AngularCompilerProgram.listLazyRoutes (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler-cli/src/transformers/program.js:156:30)
    at Function.NgTools_InternalApi_NG_2.listLazyRoutes (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@angular/compiler-cli/src/ngtools_api.js:44:36)
    at AngularCompilerPlugin._getLazyRoutesFromNgtools (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@ngtools/webpack/src/angular_compiler_plugin.js:246:66)
    at Promise.resolve.then.then (/home/travis/build/angular/angular/aio/tools/examples/shared/node_modules/@ngtools/webpack/src/angular_compiler_plugin.js:542:50)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
webpack: Failed to compile.
Build did not succeed. Please fix errors before running e2e task

@mary-poppins
Copy link

You can preview 852e63f at https://pr20306-852e63f.ngbuilds.io/.

@kapunahelewong
Copy link
Contributor Author

Hi @alexeagle I got it green!!! :D

Thank you for your help. :)

@kapunahelewong kapunahelewong changed the title docs(aio): add NgModule docs docs: add NgModule docs Jan 10, 2018
@alexeagle alexeagle added the action: merge The PR is ready for merge by the caretaker label Jan 11, 2018
alexeagle pushed a commit that referenced this pull request Jan 11, 2018
alexeagle pushed a commit that referenced this pull request Jan 11, 2018
alexeagle pushed a commit that referenced this pull request Jan 11, 2018
@alexeagle alexeagle closed this in 53f9118 Jan 11, 2018
alexeagle pushed a commit that referenced this pull request Jan 11, 2018
alexeagle pushed a commit that referenced this pull request Jan 11, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EntryComponents Not Documented
8 participants