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

[WIP] - Update Eslint Configuartion Strategy #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

id-pogni
Copy link
Contributor

I'm hoping to move toward using more standard "dot file" approach for config files. The biggest benefit of this change is that text editors will now be able to auto-format on save.

Before this is merged, I need to test how extend/overwrite the base eslint for a from the webpack.project folder.

Once this is finished and merged, next steps are to work out:

  • Add optional Prettier config
  • Create Stylelint dot-config file.
  • Create Jest dot-config config

@@ -45,20 +45,7 @@ const WEBPACK = {
* as strict as possible.
*/
const ESLINT = {
// Optional: Replace `eslint:recommended` with `eslint-config-infield` and run
// `npm install --save-dev eslint-config-infield` for stricter linting rules
extends: "eslint:recommended",
Copy link
Contributor

Choose a reason for hiding this comment

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

I purposefully placed those few lines into webpack.project so users of this Webpack setup have it easy to adjust the ESLint config in the right place. If this part gets moved into webpack.core, I suspect that people will modify the file in the webpack.core folder directly instead of making the effort of overriding the code via webpack.project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree with you and I think you are correct. After I submitted this PR, I was looking into elegant ways to have a base config in webpack.core which is extended in webpack.project.

I also think it's rare for a company to need different ESLint rules for different projects. If that is needed, then a developer could create a new project-name/.eslintrc.js/json file which extends the default.

Thank you for reviewing this Kevin! Your feedback is really appreciated. This PR isn't done, and I should have marked it as a work in progress [WIP].

@kevinweber
Copy link
Contributor

It is common practice to place the package.json, ESLint config and other configurations in the root folder of a repository. If that is done, practically all problems around resolving modules and using the right config files get resolved. I think that's also Adobe's approach with their SPA demo app; you might want to check how they're currently solving this.

Given your concerns, is it worth to move the package.json from the webpack.core folder to the very root of a project, plus also moving the remaining webpack.core folder to the root as well? The new project structure would be:

./
./ui.apps/
./ui.apps/src/main/webpack.project/
- ./ui.apps/src/main/webpack.core/package.json
+ ./package.json
- ./ui.apps/src/main/webpack.core/
+ ./webpack.core/

You can also consider moving the webpack.project folder to the root as well and make it "webpack.projects":

./
./ui.apps/
- ./ui.apps/src/main/webpack.project/
+ ./webpack.projects/
./package.json
./webpack.core

@id-pogni id-pogni changed the title Update Eslint Configuartion Strategy [WIP] - Update Eslint Configuartion Strategy Dec 17, 2018
@id-pogni
Copy link
Contributor Author

@kevinweber, Do you recall why we did not put the FE build on the root of the AEM folder?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants