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

Allow script-shell config in package.json #36

Closed
wants to merge 1 commit into from
Closed

Allow script-shell config in package.json #36

wants to merge 1 commit into from

Conversation

danielbayley
Copy link

@danielbayley danielbayley commented Jun 11, 2019

RFC proposing that script-shell be directly configurable from package.json.

Rendered

@ljharb
Copy link
Contributor

ljharb commented Jun 11, 2019

Which would take precedence if both files are defined? Wouldn’t this make npm config even slower, since for the first time it’d have to read and parse package.json, instead of just npmrc files and env vars?

@isaacs
Copy link
Contributor

isaacs commented Aug 19, 2019

My main objection to this is that it seems weird to pick out this config value as the only one to set as a project-level config in package.json.

It seems easy enough to just add a blessed package.json object that would do project-level configs. The pattern in other node tools (babel, typescript, jest, tap, and nyc, that I'm aware of) is to use the name of the tool as the field name. So it'd look something like this:

{
  "name": "my-project",
  "version": "1.2.3",
  "npm": {
    "script-shell": "/bin/bash",
    "other-thing": "blarg"
  }
}

I don't think that'd be too terrible performance-wise, but it would mean that we have to update the config handling logic to treat both ./.npmrc and ./package.json as project-level config files. And, if there is both a npm field in package.json and a project-level .npmrc file, we'd have to decide which one has higher priority (or if it's an error to have both).

@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call Enhancement new feature or improvement Needs Discussion is pending a discussion semver:minor new backwards-compatible feature labels Oct 30, 2019
@darcyclarke darcyclarke requested a review from a team October 30, 2019 05:14
@darcyclarke darcyclarke added Needs Documentation pull request requires docs before merging and removed Agenda will be discussed at the Open RFC call labels Oct 30, 2019
@isaacs
Copy link
Contributor

isaacs commented Oct 30, 2019

As discussed in open RFC meeting today, we're not going to move forward with this, and instead will evaluate #59 and #60 separately, as these would provide a more general solution to the problem identified here.

Thanks!

@isaacs isaacs closed this Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement Needs Discussion is pending a discussion Needs Documentation pull request requires docs before merging semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants