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.

Sign up for GitHub

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

Jump to bottom

Update index.ts #59

Closed
wants to merge 2 commits into from
Closed

Update index.ts #59

wants to merge 2 commits into from

Conversation

kamleshchandnani
Copy link

@kamleshchandnani kamleshchandnani commented Oct 28, 2020 •
edited
Loading

Check for npmrc in project root first then check in HOME directory. If it's not found at either place then create one in the the HOME directory.

fixes #58

@kamleshchandnani
Update index.ts
3927fec 
Check for `npmrc` in project root first then check in HOME directory. If it's not found at either place then create one in the project root instead of the HOME directory to be on a safer side and not tampering the HOME directory

fixes changesets#58
Andarist
Andarist reviewed Oct 28, 2020
View reviewed changes
src/index.ts
let npmrcPath = `${process.env.HOME}/.npmrc`;
if (fs.existsSync(npmrcPath)) {
let npmrcHomePath = `${process.env.HOME}/.npmrc`;
let npmrcProjectRootPath = `${process.env.PWD}/.npmrc`;
Copy link
Member

Choose a reason for hiding this comment

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

is there any particular reason why you have chosen this over process.cwd()?

Copy link
Author

Choose a reason for hiding this comment

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

process.env.PWD is a safer way compared to process.env.cwd() because while your process is running and if one of the commands in that process changes the working directory while it's still executing the value of process.env.cwd() will change at all the places where it's being referred. process.env.PWD guarantees that your current working directory will be set to a path when the command actually started executing and wouldn't change during the lifecycle of the process

Andarist
Andarist reviewed Oct 28, 2020
View reviewed changes
Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

LGTM in general - but I have not been using .npmrc files much so I'm not super confident myself with merging this, would prefer if @mitchellhamilton would take a look at this first

@emmatown
Copy link
Member

I'm not sure why we would want to do this?

Creating the .npmrc in the current working directory is definitely unsafe since since we commit from there and most people probably won't have .npmrc in their gitignore and people frequently have .npmrc committed in projects that set a certain registry for a scope but have their authentication set globally for example.

For the linked issue, won't npm prefer any settings set in the local .npmrc over the global .npmrc so writing the global .npmrc shouldn't cause any issues?

@kamleshchandnani
Copy link
Author

kamleshchandnani commented Oct 29, 2020 •
edited
Loading

I'm not sure why we would want to do this?

Creating the .npmrc in the current working directory is definitely unsafe since since we commit from there and most people probably won't have .npmrc in their gitignore and people frequently have .npmrc committed in projects that set a certain registry for a scope but have their authentication set globally for example.

For the linked issue, won't npm prefer any settings set in the local .npmrc over the global .npmrc so writing the global .npmrc shouldn't cause any issues?

@mitchellhamilton Sure makes sense. I'll make changes and write the .npmrc to the Home directory.

@emmatown
Copy link
Member

To be clear, I'm unsure what problem this PR solves since npm should respect config in a local .npmrc over the global .npmrc + not writing the global config if a local config exists could cause problems if people have a local .npmrc with some config committed to their repo(but without auth)

@kamleshchandnani
Copy link
Author

To be clear, I'm unsure what problem this PR solves since npm should respect config in a local .npmrc over the global .npmrc + not writing the global config if a local config exists could cause problems if people have a local .npmrc with some config committed to their repo(but without auth)

@mitchellhamilton I've an .npmrc at the project root but still since changeset checks for .npmrc in home directory and if it doesn't exists it just creates one and when it publishes it's referring to the global one and I get failure because my registry is different but the project's .npmrc is not getting picked up and I need to unnecessarily write this extra step in my action.

- name: Creating .npmrc
  run: |
    cat << EOF > "$HOME/.npmrc"
      [email protected]
      //registry.npmjs.org/:_authToken=$NPM_TOKEN
    EOF
  env:
    NPM_TOKEN: ${{ secrets.NPM_TOKEN }}

@emmatown
Copy link
Member

the project's .npmrc is not getting picked up

Hmmmm, I think that'll be because of changesets/changesets#448. I don't think changing what's being changed in this PR would fix that.

@kamleshchandnani
Copy link
Author

Oh okay alright! maybe I'll close this then.

@kamleshchandnani kamleshchandnani deleted the patch-1 branch October 29, 2020 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andarist Andarist Awaiting requested review from Andarist

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

project's .npmrc not respected
3 participants
@kamleshchandnani @emmatown @Andarist