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

Add configFileMode option #158

Merged
merged 5 commits into from Nov 21, 2021

Conversation

ncallaway
Copy link
Contributor

This is a proposed implementation for #157. This adds a mode option, which is passed through to the underlying call to fs.writeFileSync or atomically.writeFileSync.

The default mode here (0o666) is the default mode for both fs.writeFileSync and atomically.writeFileSync so this should be a backwards-compatible change.

source/types.ts Outdated

@default 0o666
*/
mode: number | string;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
mode: number | string;
configFileMode: number;

I'm aware the underlying API supports a string, but that's only for legacy reasons. I prefer to keep it simple.

source/types.ts Outdated
@@ -137,6 +137,13 @@ export interface Options<T> {
*/
clearInvalidConfig?: boolean;

/**
The mode that will be used for the config file.
Copy link
Owner

Choose a reason for hiding this comment

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

Linkify mode to a website that explains what mode is. Not every user is familiar with the concept.

source/types.ts Outdated
@@ -137,6 +137,13 @@ export interface Options<T> {
*/
clearInvalidConfig?: boolean;

/**
The mode that will be used for the config file.

Copy link
Owner

Choose a reason for hiding this comment

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

The docs should explain when it would be useful. Most devs would not need this, and from experience, setting mode can cause a lot of problems. For example, user run a tool with sudo and then later not with sudo and then the config file is not readable, etc. I did the mode thing in a different config library, and it caused endless of support requests.

For example, on macOS, conf by default, stores the config in ~/Library/Preferences, which is not world writable. So this would not be needed if you target only macOS.

https://github.com/sindresorhus/env-paths#pathsconfig

@sindresorhus
Copy link
Owner

You need to document it in the readme too.

@ncallaway
Copy link
Contributor Author

I'll make the changes requested, and the CI formatting fixes shortly. The only question I have is the proper destination for the mode link.

The node documentation for fs.open() (where they describe mode in the most detail) links to https://man7.org/linux/man-pages/man2/open.2.html . Another option would be the wikipedia page for File system permissions numeric notation section (https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation), or other more casual explainer: https://docs.nersc.gov/filesystems/unix-file-permissions/.

Do you have any preferences for the level of detail in the explanation link for the mode options?

@ncallaway
Copy link
Contributor Author

I've pushed a change as a separate commit that does the following:

  • Renames the parameter from mode to configFileMode
  • updates the type definition to only allow number for configFileMode
  • updates the documentation of mode with more details (including a link to the wikipedia link above https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation)
  • adds the mode documentation to readme.md
  • updates the formatting to pass npm run test

This is a separate commit to make it easy to see the changes from before, but I'm also happy to squash into a single commit before this is merged.

@sindresorhus sindresorhus changed the title Add mode option Add configFileMode option Nov 21, 2021
@sindresorhus sindresorhus merged commit 8345d71 into sindresorhus:main Nov 21, 2021
@sindresorhus sindresorhus mentioned this pull request Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants