Problem/Motivation

The Eslint configuration is set up with only a few rules at the moment. This means it's not catching things like:

  • Comments well over 80 chars
  • Inconsistency in single vs double quotes
  • Inconsistency in lines ending with semi colons
  • Inconsistency in trailing commas

Obviously none of these things are bad and we're still in rapid iteration mode, but to save us pain in the future I think it's worth setting up some rules now.

Steps to reproduce

Review the codebase

Proposed resolution

Add the following to eslintrc

In extends, add "plugin:prettier/recommended". We already have this installed

Add some basic prettier rules

"prettier/prettier": ["error", {
      "singleQuote": true,
      "trailingComma": "all",
      "arrowParens": "avoid",
    }],

Set max line length and brace-style

"max-len": "error",
    "brace-style": ["error", "stroustrup"]

Alternatively, if we wanted to go even further we could embrace cores eslint, which is even more strict

Remaining tasks

Add changes to MR (Incoming)
Agree on approach
Fix the lint issues introduced as part of this MR
Merge

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork experience_builder-3452828

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DanielVeza created an issue. See original summary.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Issue summary: View changes

DanielVeza opened merge request !44

Wim Leers credited larowlan.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Update eslint prettier configuration » [PP-1] Update eslint prettier configuration
Status: Active » Postponed
Parent issue: » #3450592: [META] Front-end Kanban issue tracker
Related issues: +#3450307: CI: use template's `eslint` job to validate YML and JS, add new `UI eslint` job for UI's TypeScript

Thanks, @DanielVeza! Excited to see you here! 😄

Per @larowlan, we should first land #3450307: CI: use template's `eslint` job to validate YML and JS, add new `UI eslint` job for UI's TypeScript to avoid the need to redo this in the future.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Update eslint prettier configuration » Update eslint prettier configuration
Status: Postponed » Needs work

#3450307: CI: use template's `eslint` job to validate YML and JS, add new `UI eslint` job for UI's TypeScript is in, meaning this is now unblocked!

Mithun S made their first commit to this issue’s fork.

Gauravvvv made their first commit to this issue’s fork.

Gauravvvv opened merge request !77

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
finnsky’s picture

finnsky commented

I've found MR contains some unexpected changes.
Like src/App.tsx

Also lint:fix show failures.

finnsky’s picture

finnsky commented
Status: Needs review » Needs work
finnsky’s picture

finnsky commented

I also see existing `"format": "prettier --write .",` task in package.json

Should we keep it?

finnsky changed the visibility of the branch 3452828-prettier to hidden.

finnsky opened merge request !78

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

I've created new PR.

- It should only affect js changes.
- We need to create follow up issue for stylelint and yaml etc. Because current `npm run format` still fixes public and MD and CSS files.

bnjmnm made their first commit to this issue’s fork.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work

There were some pipeline failures that I was looking into only to re-run the tests and they were fine. In each case it was because the project directory could not be found, something that would not be caused by changes in either of the merge requests.

While I'm partial to the @finnsky MR #78 because it only changes the JS, the npm run format command still runs on non-js files. This means someone wanting to use format will have files changed that they haven't even edited. We should either include all file types or limit the types changed by format

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@finnsky: Could you address #18? 😊 🙏 Would love to see this land!

finnsky’s picture

finnsky commented

I have weird deja vu :)

https://www.drupal.org/project/drupal/issues/3409048#comment-15525622
Linter issues aren't rebase friendly.

I will reapply changes in new 0.x based branch and let's create followup for CSS and MD

finnsky changed the visibility of the branch 3452828-eslint-prettier to hidden.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: Unassigned » wim leers
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs review » Needs work

#20: hah!

I'm fine with doing additional things in follow-ups, but would like to see @bnjmnm's feedback in #18 addressed. That doesn't require Doing All The Things in this issue — an alternative can be to restrict what npm run format does. Could you do that, @finnsky? 🤞

finnsky’s picture

finnsky commented

Removed format for now. We need to configure linter + prettier for css and other files.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » bnjmnm

@bnjmnm you did the last round of review for this in #18 — could you give this another pass? 🙏

bnjmnm opened merge request !124

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » hooroomoo

Removed format for now

Not sure that's a good idea either - if GitlabCI + eslint is enforcing prettier formatting, we shouldn't require contributors to fix all of those things manually when a formatter is right there I think it's best to go with my suggestion in #18 and have the formatter only act on JS files for the time being, while also configuring ESLint to enforce prettier formatting. That's what I have in the <a href="https://git.drupalcode.org/project/experience_builder/-/merge_requests/124#note_349548">3452828-js-only-eslint-with-format </a> MR

  • bnjmnm committed 9540e1ee on 0.x 
    Issue #3452828 by finnsky, bnjmnm, Gauravvvv, DanielVeza, larowlan,...

bnjmnm credited hooroomoo.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: hooroomoo » Unassigned
Status: Needs review » Fixed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

OMG YES! So glad this is in! 😄👍

Wim Leers closed merge request !78

Wim Leers closed merge request !44

Wim Leers closed merge request !77

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.