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 support for linting json files #3350

Closed
wants to merge 3 commits into from

Conversation

viankakrisna
Copy link
Contributor

@viankakrisna viankakrisna commented Oct 30, 2017

Verified the changes by creating invalid .json file in src and importing it via index.js

{
    some: 'setting'
}

Before:

screenshot from 2017-10-30 14-35-27

After:

screenshot from 2017-10-30 14-34-32

@viankakrisna
Copy link
Contributor Author

@Timer seems like this PR doesn't trigger @react-scripts-dangerous, any reasons why?

@Timer
Copy link
Contributor

Timer commented Oct 31, 2017

It hasn't ran in a few days, I might of shut it off on Heroku -- I'm not sure what's up 🤔.

Will probably look at it here soon again because it needs to be working to cut next releases.

@viankakrisna
Copy link
Contributor Author

@gaearon @Timer @tharakawj will this be merged?

@Timer
Copy link
Contributor

Timer commented Nov 10, 2017

I do like this; we'll schedule it for 2.0.0 just in case there's some weird edge case that breaks JSON in the plugin.

@Timer Timer added this to the 2.0.0 milestone Nov 10, 2017
@Timer Timer changed the base branch from master to next November 10, 2017 16:32
@@ -149,6 +149,12 @@ module.exports = {
// match the requirements. When no loader matches it will fall
// back to the "file" loader at the end of the loader list.
oneOf: [
// This is included by default, but when we add .json to eslint-loader
// test regex the compiler fails to recognize json files.
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit weird. Can we figure out why this happens and if there's a nicer way to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

webpack-contrib/json-loader#44 which links to https://webpack.js.org/guides/migrating/#json-loader-is-not-required-anymore

When no loader has been configured for a JSON file, webpack will automatically try to load the JSON file with the json-loader.

I think adding json test in eslint-loader counts as loader has been configured for a JSON file

@viankakrisna
Copy link
Contributor Author

@gaearon added a link in the comments about why we need json-loader

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

My main concerns are:

  1. More complicated post-eject configuration (given that webpack finally handles json by default and it's annoying to have to special-case it again)
  2. Performance. If you import huge JSON files, how does this affect the build performance?

@viankakrisna
Copy link
Contributor Author

  1. Yeah, but i think it's a fair tradeoff concerning people that got bitten by debugging json file, when it's big enough and you only given the position is when we needed it the most :)

  2. build time comparison between next and json-lint with json file that has 18964 lines

screen shot 2018-01-17 at 01 26 35

so, yeah, i think this is a no 😄

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

That's impacting the build time too much unfortunately.
Maybe there's some other way we can fix it, e.g. by using a better parser in webpack itself?

@gaearon gaearon closed this Jan 16, 2018
@viankakrisna
Copy link
Contributor Author

yeah i thought so, eslint-plugin-json itself is using jshint(!) internally. I'll dig more on the webpack side when i found the time.

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants