jschr / electron-react-redux-boilerplate

A minimal boilerplate to get started with Electron, React and Redux.
556 stars 130 forks source link

Restructure the app to cascade eslintrc configuration #61

Closed pronebird closed 6 years ago

pronebird commented 6 years ago

Rationale

I'd like to have a discussion regarding eslint configuration and potential changes in the file structure.

This PR is only an example of how we can go about the restructuring of the app in order to have a proper configuration for each environment of the app.

There are three separate environments that we maintain within the boilerplate:

  1. Main electron process
  2. Renderer electron process
  3. Test environment that's basically mocha + renderer electron process

Currently we don't distinguish those environments in our .eslintrc configuration and basically enable mocha and browser related features and globals for the entire source tree.

This is apparently wrong, since browser features are only available in the renderer process, and mocha is only automatically injected in tests.

Solution

One way to fix this is to cascade the .eslintrc configurations, by having one base "root" config and nested .eslintrc in sub-folders that tweak certain configuration values based on the environment that the code is expected to run in. The code itself has to be restructured as well.

Changelog

This section briefly describes the file changes.

If you have any ideas how to better organize the file structure, I am all ears. Also I don't like app/main/main.js, that main twice, ugh.. But I can't come up with anything better right now, hence I am looking for some feedback :)

Moved
Added
Modified

Ps: Please, when merging PRs in the future, rebase then merge, this way the Git history remains more linear and less intertwined. Github's automatic merge supports the option to "Rebase and merge"

jschr commented 6 years ago

d'oh. Saw this pr last. I'm used to squash and merge but will rebase in the future!

Overall, I like these changes. Will think about it a bit more though!

jschr commented 6 years ago

I'm fine with main/main.js. As an alternative, we could rename to main/index.js– in which case we wouldn't need to modify init.js.

Feel free to merge if you're happy with the changes.

It might be worth updating the README with a blurb about the structure / eslint configs.

pronebird commented 6 years ago

@jschr well actually I am not sure anymore haha. I used to merge manually by doing:

git checkout feature
git rebase master
git checkout master
git merge --no-ff feature

But it seems that "Rebase and merge" does something else. 😄

See here https://github.com/isaacs/github/issues/1143

pronebird commented 6 years ago

Just renamed main/main.js -> main/index.js as per your suggestion. I think it's better, thanks! 👍