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

[react-scripts] Do not lint *.bs.js files, generated by BuckleScript #3714

Conversation

emmenko
Copy link

@emmenko emmenko commented Jan 8, 2018

Summary

This PR introduces a small change in the eslint rule of webpack.config to exclude files ending with the suffix *.bs.js (which are compiled files generated by BuckleScript).

NOTE: I'm no RegEx expert, so if someone has a better suggestion about the test rule feel free to leave a comment 🙏

Background

When working on a React project which includes fully or partially ReasonML, you need BuckleScript to generate the compiled JS sources. When you have the in-source option active you will get the compiled JS sources (*.bs.js) next to the Reason source file.
Those files will therefore "live" inside the src folder, causing eslint to print some warnings:

image

This is a bit annoying and can simply be "solved" by telling eslint to ignore those files.

Wait, ReasonML? What about reason-scripts?

As you might know, there is a package called reason-scripts which is basically a "clone" of react-scripts, with some things adapted to compile Reason files out of the box (e.g. entry point index.re instead of index.js).

However, as I pointed out in this issue, I don't think it's necessary to have reason-scripts in the first place.
@rrdelaney also pointed out (correctly if I'm wrong) that bs-loader should not be included in the package anymore.
In fact, you can run the BuckleScript compiler in watch mode as a parallel process, next to react-scripts start. This way you don't need the loader at all and you can simply use the "normal" react-scripts package.

"scripts": {
  "start": "npm-run-all --parallel start:bsb start:app",
  "start:app": "react-scripts start",
  "start:bsb": "bsb -make-world -w",
}

Anyway, this is another story...

Btw, this change will need to be made in reason-scripts as well.


Hopefully this gives you enough background/context to understand this change.
I'm open to any feedback/suggestion.

Thanks 🙏

@emmenko emmenko force-pushed the react-scripts-exclude-bs-files-from-eslint branch from 35f96b4 to fe5f598 Compare January 8, 2018 20:28
@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

I don't think it makes sense for all people who eject to end up with this configuration as it serves a very narrow use case.

@emmenko
Copy link
Author

emmenko commented Jan 8, 2018

I understand. However there is no way (as far as I know) to use a .eslintignore as it will get ignored (see #2115).

Do you have any other suggestion to tackle this case?

@emmenko
Copy link
Author

emmenko commented Jan 8, 2018

Btw, if that's the only concern I guess we can simply annotate those extra lines with @remove-on-eject-begin / @remove-on-eject-end 🤔

Update: if you still use Reason and eject you would need to "re-add" those lines back in, so it doesn't help much anyway.

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

I don't really see us doing this. The config is already complex enough after ejecting, and we need to be very careful about adding things there. Instead I'd suggest doing something to automatically inject // eslint-ignore or similar.

@gaearon gaearon closed this Jan 9, 2018
@emmenko
Copy link
Author

emmenko commented Jan 9, 2018

Hmm so is react-scripts not meant to be used for Reason projects?

@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.

3 participants