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: update npm packages to be accurate for v7 #26422

Closed
wants to merge 8 commits into from

Conversation

jenniferfell
Copy link
Contributor

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?

Npm packages doc was not current for v7.

  • Referred to old version of node
  • Described yarn as default for CLI, not npm
  • package lists were not current

Issue Number: N/A

What is the new behavior?

The doc needs a larger rewrite, but this PR is to make it accurate in the ways mentioned above.

Also made some copy edits.

Does this PR introduce a breaking change?

[ ] Yes
[X ] No

Other information

@jenniferfell jenniferfell added comp: docs target: major This PR is targeted for the next major release labels Oct 12, 2018
@jenniferfell jenniferfell added this to the v7.0 milestone Oct 12, 2018
@jenniferfell jenniferfell added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 12, 2018
aio/content/guide/npm-packages.md Outdated Show resolved Hide resolved
aio/content/guide/npm-packages.md Show resolved Hide resolved
aio/content/guide/npm-packages.md Outdated Show resolved Hide resolved
@@ -155,19 +147,18 @@ Built on top of [WebDriverJS](https://github.com/SeleniumHQ/selenium/wiki/WebDri
the TypeScript language server, including the *tsc* TypeScript compiler.


## So many packages! So many files!
## Managing packages and files

The default `package.json` installs more packages than you'll need for your project.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an odd statement. The CLI scaffolds the project with the minimum amount of packages needed.

@@ -155,19 +147,18 @@ Built on top of [WebDriverJS](https://github.com/SeleniumHQ/selenium/wiki/WebDri
the TypeScript language server, including the *tsc* TypeScript compiler.


## So many packages! So many files!
## Managing packages and files

The default `package.json` installs more packages than you'll need for your project.

A given package may contain tens, hundreds, even thousands of files,
all of them in your local machine's `node_modules` directory.
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 scary. remove

aio/content/guide/npm-packages.md Outdated Show resolved Hide resolved
aio/content/guide/npm-packages.md Show resolved Hide resolved
aio/content/guide/npm-packages.md Outdated Show resolved Hide resolved
aio/content/guide/npm-packages.md Show resolved Hide resolved
aio/content/guide/npm-packages.md Show resolved Hide resolved
@mary-poppins
Copy link

You can preview 5b38230 at https://pr26422-5b38230.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 697ceab at https://pr26422-697ceab.ngbuilds.io/.

@mary-poppins
Copy link

You can preview bdbe716 at https://pr26422-bdbe716.ngbuilds.io/.

@IgorMinar IgorMinar added target: patch This PR is targeted for the next patch release action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 16, 2018
@IgorMinar
Copy link
Contributor

there are still many unresolved comments on this pr

@mary-poppins
Copy link

You can preview 4e3b615 at https://pr26422-4e3b615.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 2de0e55 at https://pr26422-2de0e55.ngbuilds.io/.

@jenniferfell jenniferfell removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 17, 2018
@mary-poppins
Copy link

You can preview b668c2d at https://pr26422-b668c2d.ngbuilds.io/.

@jenniferfell
Copy link
Contributor Author

jenniferfell commented Oct 17, 2018

@IgorMinar : Ready for another look.

  • Q1: is package.json workspace-scope or project-scope? I see that ng new creates a package.json at the workspace level. I see that ng generate app does not create a package.json in my new app project under projects.

    Current text: ”The CLI ng new command creates a default package.json file for your project.”

    Seems like it should either be that ng new creates a package.json for your workspace, or that both ng new and ng generate app create package.json when they create projects?

  • Q2: Page title “Angular packages and dependencies” and nav title “Packages and dependencies” ? OR “Angular package dependencies” and “Package dependencies” ? Just trying something other than “npm packages” if you have a quick pref to change.

  • Q3: Are these the best URLs for the devDependencies that begin @angular?

Thanks!

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

A1: workspace scoped
A2: neither is great. packages are dependencies. so we just repeat the same thing using two different words. how about: “npm dependencies” and “Workspace npm dependencies”?
A3: left in inline comment. the rest looks good.

aio/content/guide/npm-packages.md Outdated Show resolved Hide resolved
@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 18, 2018
@IgorMinar
Copy link
Contributor

I'm going to approve this because it's almost done. Let's change the title, resolve the other issues and get this in.

@mary-poppins
Copy link

You can preview d1eb8bd at https://pr26422-d1eb8bd.ngbuilds.io/.

@jenniferfell
Copy link
Contributor Author

jenniferfell commented Oct 19, 2018

Note that ng g library does however create a package.json for the library, but it's really there for publishing the library to NPM. It's not something that would be npm installed in the project dir. I'm sure that the library details are covered elsewhere in the docs and doesn't need to be part of this page.

@Splaktar Thanks for this tip. I don't think it's covered in the AIO docs, and I don't see it clearly stated on the CLI wiki. In general, we're trying to do a better job of including information about library development (we have a few top-of-backlog items), so I included a brief mention and link to ng generate doc. We'll need to add more to that doc and address the backlog need for guide material, but I do think this is the right place to mention that libs have package.json files. Thanks again!

@jenniferfell jenniferfell removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 19, 2018
@mary-poppins
Copy link

You can preview 296f04f at https://pr26422-296f04f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview a368f7c at https://pr26422-a368f7c.ngbuilds.io/.

@jenniferfell
Copy link
Contributor Author

@IgorMinar Here, again. I changed navigation.json so now the PR needs a fresh approval from someone in the marketing group. Thx.

@jenniferfell jenniferfell added the action: merge The PR is ready for merge by the caretaker label Oct 19, 2018
@jenniferfell
Copy link
Contributor Author

Other than that approval, I think it's ready. I've incorporated all review feedback, updated and added cross-reference links, and moved the lists into tables that are easier to read. (When I added links, scanning the in-line titles became harder. The table fixes that.)

Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@mary-poppins
Copy link

You can preview d9415fb at https://pr26422-d9415fb.ngbuilds.io/.

alxhub pushed a commit that referenced this pull request Oct 23, 2018
@alxhub alxhub closed this in 26e8032 Oct 23, 2018
@jenniferfell jenniferfell deleted the jfell-node-version branch October 24, 2018 17:45
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@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 14, 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 cla: yes effort2: days risk: low target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants