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

added a header for usage with process.env #14

Merged
merged 3 commits into from Aug 7, 2018
Merged

Conversation

@mwarger
Copy link
Contributor

@mwarger mwarger commented Jul 17, 2018

added a header for usage with process.env to separate section from 'current lifecycle event' and make it easier to find

added a header for usage with process.env to separate section from 'current lifecycle event' and make it easier to find
@mwarger mwarger requested a review from as a code owner Jul 17, 2018
Copy link
Contributor

@zkat zkat left a comment

I'm not really sure this header is needed -- reading this section a few times, it seems clear that the section under your header is directly relevant to its original location, and I think I'd much prefer keeping the docs as they are, in this case?

@zkat zkat added the docs label Jul 18, 2018
@terinjokes
Copy link
Contributor

@terinjokes terinjokes commented Jul 18, 2018

The Twitter thread that spawned this was in relation to the "package.json vars" section, and the lack of a usage example.

If we're going to be changing the docs, I agree with @zkat that creating a new header here doesn't make sense.

Perhaps add an example to the other section showing off npm_package_name or npm_package_version?

@mwarger
Copy link
Contributor Author

@mwarger mwarger commented Jul 18, 2018

I added the header because that paragraph (the one I put the header above) seemed to apply to all the previous paragraphs. The flattening of the object structure with the subsequent description of how to use it.

That's a good point, though - I will try your suggestion. Thank you for the feedback.

@zkat zkat requested a review from neverett Jul 23, 2018
@zkat zkat removed the in-progress label Jul 23, 2018
`npm_package_version` set to "1.2.5"
`npm_package_version` set to "1.2.5".

#### Usage
Copy link
Contributor

@neverett neverett Jul 25, 2018

For consistency with the other sections, I would recommend moving the example up to follow the previous paragraph, and changing the language a bit. Here's how that might look:

The package.json fields are tacked onto the npm_package_ prefix. So, for instance, if you had {"name":"foo", "version":"1.2.5"} in your package.json file, then your package scripts would have the npm_package_name environment variable set to "foo", and the npm_package_version set to "1.2.5". You can access these variables in your code with process.env.npm_package_name and process.env.npm_package_version, and so on for other fields.

Copy link
Contributor

@neverett neverett left a comment

One comment on the placement and wording of the example -- let me know what you think.

@mwarger
Copy link
Contributor Author

@mwarger mwarger commented Jul 26, 2018

Good call - I made that change. Thanks all for the feedback.

@mwarger
Copy link
Contributor Author

@mwarger mwarger commented Jul 27, 2018

This was just a text change - can the CI be kicked off again? I don't think my change caused this...

zkat
zkat approved these changes Jul 30, 2018
Copy link
Contributor

@zkat zkat left a comment

👍 changes look good to me. Thank you!

And don't worry about CI. It's not your fault ^_^;;

@zkat zkat changed the base branch from latest to release-next Jul 30, 2018
@zkat zkat removed the in-progress label Aug 7, 2018
@zkat zkat merged commit e2346e7 into npm:release-next Aug 7, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@travis-ci[bot]
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants