-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Move babel and eslint configuration to package.json #405
Conversation
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "react-slingshot", | |||
"version": "5.0.0", | |||
"version": "7.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the big version hike?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well embarrassingly, when I announced 6.0, I didn't update package.json to reflect it. I would consider the config move a breaking change, so that's why I went to 7. Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I forgot the current release was 6.0, so I was mostly trying to figure how one change was 2 version. All good by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am good with this. I know this came up a while back on twitter. My only concern is size of the package.json is have many purposes now.
May be a good next step to add a script that moves it from package.json to rc files. This actually may be good as a stand alone packaged-cli tool.
I'm fine with the change as well. Might as well stop causing headaches for Windows users. |
The size of package.json wouldn't be bad if we dropped all the custom eslint rules. I'd like to consider dumping all custom rules and just use eslint's recommended as a basline. Thoughts on that? |
I'm OK with that.
On Tue, Apr 4, 2017 at 9:36 AM Cory House <notifications@github.com> wrote:
*@coryhouse* commented on this pull request.
------------------------------
In package.json
<#405 (comment)>
:
@@ -1,6 +1,6 @@
{
"name": "react-slingshot",
- "version": "5.0.0",
+ "version": "7.0.0",
Well embarrassingly, when I announced 6.0, I didn't update package.json to
reflect it. I would consider the config move a breaking change, so that's
why I went to 7. Make sense?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#405 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAy2z7j58eKvTR4zJ-RcclVxcUzwe1fYks5rskc4gaJpZM4My1Hv>
.
|
I agree for the purposes of this repo a baseline/standard rule list may be better suited. Any updates we can make to the babel config to make that cleaner? Maybe introduce |
@coryhouse, I'll take a peek tonight at your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coryhouse, I grabbed your PR locally and ran it. We're all good to go.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "react-slingshot", | |||
"version": "5.0.0", | |||
"version": "7.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
@kwelch anything else to add before I merge? |
I am good, I think there are few action items from this around the lint and babel configs. Should we open an issue for those to discuss? |
@kwelch, yeah, can you open another issue for that? |
It's easy to overlook .babelrc and .eslintrc when copying files, especially on Windows. So I think it makes sense to move them to package.json. Many other boilerplates use this approach as well.
What does everyone think?