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

Update Node.js README file #6664

Merged
merged 5 commits into from Oct 31, 2018

Conversation

Projects
None yet
3 participants
@nsaechao
Collaborator

nsaechao commented Oct 20, 2018

The README file has not been updated in a long time. Make small adjustments to include yarn package manager and source-level dependencies.

Ny Saechao

@baroquebobcat baroquebobcat self-requested a review Oct 22, 2018

@stuhood

Thanks a lot for these changes!

May I recommend including this in the top-level docsite? To see how to do that, grep for go_readme to see how it is included:

$ rg go_readme
contrib/go/BUILD
5:  name='go_readme',

src/docs/docsite.json
32:    "go_readme": "dist/markdown/html/contrib/go/README.html",
167:       {"pages" : ["go_readme"]},

Once you've linked the page into the docsite, you can preview it with build-support/bin/publish_docs.sh -o (see https://www.pantsbuild.org/docs.html for more info).

in the source tree would need its dependencies duplicated in the BUILD target and package.json file.
Obviously this is not desirable. Over time, Pants support for NPM should improve to work with a
fully functional package.json with no duplication of information in the BUILD file.
`node_module`: A node module target.

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

Markdown will not render these as a list unless you prefix them with either bullets or numbers (they get mashed into a single paragraph).

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

If you preview the docsite, or use the github-builtin markdown viewer (https://github.com/pantsbuild/pants/blob/981d66ff4114d8a1f2da74423db35ef936355a7d/contrib/node/README.md), you can check out how this renders.

# Functionality
Utility is still limited right now.
There is limited support for `lint` and `fmt` that leverage the use of `eslint`. `eslint` global rules will need to be configured by the repo owner
and currently does not support target-level rule overrides.

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

s/does/do/

Pants will bootstrap itself with its own copy of Node and NPM and use them to run commands.
Source-level dependencies is currently supported within the `yarn` package manager. To specify a target-level dependency on

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

s/is currently/are currently/

your dependencies in your BUILD target definition.
NPM 5.0+ and Yarn both support locking dependencies to specific versions through the `package-lock.json` and `yarn.lock` files respectively.
To ensure determinism and reproducibility within Pants, these files should be checked into your repository and included in the the target definition.

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

s/the the/the/

You can resolve node_remote_module and node_module targets. Pants effectively walks forward from
the edges of the dependency tree topological sort for a target and does an "npm install"
`./pants resolve [target]`

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

Thank you for cleaning this section up... but I wonder whether you shouldn't fully replace the suggestion to run resolve with a suggestion to run node-install? I can't think of a reason to manually run resolve: folks would probably want to run test instead?

under the working directory. Targets resolved into the .pants.d directory using the yarn package manager
will use the `--frozen-lockfile` parameter and will deterministically install your dependencies.
Pants can install Node modules into the source definition directory with

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

Would expand this section.

## REPL
"./pants repl [target]" lets you run Node in REPL mode from the resolved target's path under the
working directory.
REPL is only working with npm package manager.

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

s/is only working/only works/

@@ -58,10 +86,14 @@ The examples directory contains a few types of interdependent JS projects.
containing its own test file
* A web project using React, built with the web build tool, depending on the server project and
button component, and containing its own test file
* A yarn workspace project using source-level dependencies in two ways. (Workspaces and pants).
Source-level dependencies does not need Yarn Workspaces.
Resolving the server project produces a dist directory with code transpiled by Babel, in addition

This comment has been minimized.

@stuhood

stuhood Oct 22, 2018

Member

You could maybe move these trailing paragraphs into nested bullets?

@baroquebobcat

Thanks. This is looking much better. I've got a couple wording suggestions.

you can specify node_module targets defining projects in the source tree.
Node.js targets in pants are designed to work with existing Node.js package management tools. A node_module target will work with a package.json
source file in the same directory, combining the BUILD dependencies with the additional fields from the source package.json to form the complete
package.json used for tasks on the target.

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 23, 2018

Contributor

I found this paragraph a little confusing to follow. I took a crack at rewording it. What do you think?

Node.js targets in Pants are designed to work with existing Node.js package management tools and
their configuration files. The node_module target type uses a package.json file in the same
directory as its definition. It combines dependencies declared on the target in the BUILD
file with the original package.json to form a complete package.json that can be used to run tasks
on the target through Pants.
# Functionality
Utility is still limited right now.
There is limited support for `lint` and `fmt` that leverage the use of `eslint`. `eslint` global rules will need to be configured by the repo owner

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 23, 2018

Contributor
Suggested change Beta
There is limited support for `lint` and `fmt` that leverage the use of `eslint`. `eslint` global rules will need to be configured by the repo owner
There is limited support for linting and formatting using the `lint` and `fmt` goals, which use `eslint`. `eslint` global rules will need to be configured by the repo owner
your dependencies in your BUILD target definition.
NPM 5.0+ and Yarn both support locking dependencies to specific versions through the `package-lock.json` and `yarn.lock` files respectively.
To ensure determinism and reproducibility within Pants, these files should be checked into your repository and included in the the target definition.

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 23, 2018

Contributor

I think it'd also be good to refer to the kwarg the lock file should be attached to. Maybe included in the target definition's sources argument or something.

@stuhood

This comment has been minimized.

Member

stuhood commented Oct 30, 2018

Ping! Would love to have this in.

@nsaechao

This comment has been minimized.

Collaborator

nsaechao commented Oct 30, 2018

Ping! Would love to have this in.

Sorry, looks like I lost track of this one. Will address the reviews and open a branch shortly

@stuhood

stuhood approved these changes Oct 30, 2018 edited

Looks great! Thanks.

@baroquebobcat

👍

@stuhood

This comment has been minimized.

Member

stuhood commented Oct 31, 2018

Unrelated flakes. Merging.

@stuhood stuhood merged commit 8b8d5b3 into pantsbuild:master Oct 31, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment