Create-react-app: Option not to fail build on warnings in CI environment

[varnav]

Currently if CI environment variable is set to true, and any warnings emitted, build fails (https://github.com/facebookincubator/create-react-app/pull/944). There is no way to alter this behavior other than unsetting CI envvar that may be undesirable. You cannot even customize the rule set (https://github.com/facebookincubator/create-react-app/issues/808).
Please add command-line option like --no-fail-on-warnings or add any other way to make this less strict.

https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/scripts/build.js#L115

[gaearon]

What warnings specifically do you want to ignore?

[varnav]

We are at the early stages of development of UI, and warning that annoys us most is 'jsx-a11y/alt-text' about no alt inside img. We intentionally don't have these for now.

But I would prefer just to have control over ESlint ruleset.

[Timer]

@varnav you can disable eslint warnings by adding the lines suggested to you in your development console while running npm start.

[varnav]

I don't run npm start during CI deployment process, and I don't want to disable all warnings. I want:

1. Option not to fail build on warnings inside CI environment
2. Control over eslint ruleset

[Timer]

You can place //eslint-disable jsx-a11y/alt-text (or similar) at the top of the file to only disable that warning (and not all warnings). This has the effect in development and production builds, I was simply referring to development as it provides instructions for you to do so.

[varnav]

It works as workaround, but I will have to change about 40 files. It would be better to have centralized control over this.

[Timer]

Unfortunately you cannot have control of this unless if you eject; being mindful of accessibility when you are creating images doesn't take very long.

Is there something wrong with adding alt="TODO" when you create an image? This makes it very easy later on to find where you need to fix these problems, rather than ignore it all together.

If you explicitly would like to not provide an alt, alt="" will mute the warning and is also relatively searchable.

[varnav]

You probably right, but in reality it looks different. I don't write js, I'm admin and I do DevOps things. And I have chats like this:

Programmers: Hey, build of our code fails in your CI!
Me: It's because you have warnings
P: But backend guys have warnings too, their build is ok. What can we do?
M: Fix warnings
P: We have tasks, we have deadlines, we can't do this, it's 250 lines in 40 files!
M: Ok, then add this to mute warnings to your files
P: All 40 files? We have tasks, we have deadlines. We gonna complain to your supervisor!
M: Sorry guys, I don't have control over fails/not fails on warnings, and over eslint ruleset too!
P: Yes, it's exactly what we gonna tell your supervisor. You don't have control over CI builds that are your direct responsibility!

[Timer]

FWIW this has been discussed before and warnings purposely fail CI builds by default. This is good practice.

Disabling this functionality is normally done by setting CI=false (as you mentioned in your first post); why is this not a viable option?

To clarify, setting CI=false is already the official solution to this / the option you are looking for.
If you can elaborate why this option is not good enough for you, we are more than happy to consider adding another solution.

[varnav]

This is strange, because we have 5 more different languages built in CI, and nowhere else build fails on warnings.
Setting CI to false does not help, only unsetting does. But is this a good idea? Is this variable used somewhere else?

[Timer]

Setting CI to false does not help, only unsetting does.

If this is the case, then this is a bug. Setting CI to be false should disable warnings.

But is this a good idea? Is this variable used somewhere else?

As described in Advanced Configuration, setting CI only makes npm test non-watching and makes npm run build warnings failures. There are no other side-effects, so this is completely safe to do.

Make sure you're only doing this for the npm run build command, however. You probably should not unset it in your entire test/CI environment.

[Timer]

It seems that setting CI to false will indeed still treat the variable as true.

As a temporary work-around, you can set CI to be blank via CI="" npm run build in your test script.

We're happy to accept a PR that looks for the value 'false'.

[Zaccc123]

I could do this

[Timer]

@Zaccc123 I'd like to give @varnav a change if he'd like to, can you check back tomorrow please? 😄

[Zaccc123]

Sure 😁

[varnav]

Um, as I said I don't write JS. ;)

I was trying to fix that bug myself, but ran into problems.

if (process.env.CI === 'true') will be false if CI = 'True'.

if (process.env.CI.toLowerCase() === 'true') will fail if undefined.

BTW, this tiny bug is not the main concern for me.

[Zaccc123]

I think if (process.env.CI.toLowerCase() === 'true') will fail if undefined is good, since the default behaviour of not setting it should not enter the if statement.

[varnav]

It fails the script with exception. TypeError: Cannot read property 'toLowerCase' of undefined
I guess CI environment variable is undefined on most systems.

[Zaccc123]

if (process.env.CI && process.env.CI.toLowerCase() === 'true')

You could do this to check if is defined first. If is not, it will not run the next condition and therefore no error will be thrown.

[gaearon]

Seems like this was fixed in #2501. Closing.

[varnav]

I don't really think that it's ok to close this issue, as "Option not to fail build on warnings in CI environment" still does not exist.

[gaearon]

I don't think we'll be adding an option for this. But we're happy to address individual warnings that "shouldn't be warnings" on a case by case basis. If you see an unnecessary warning please file an issue.

[bompi88]

This is not a preferable solution when a lot of frameworks and libraries depend on the CI variable in different ways. My build now fails because of named imports, which are not in alphabetical order.

[Timer]

We do not enforce a rule about alphabetical ordering -- it sounds like you have your own rules in place.

If you have an problem, please file a new issue. Thanks!