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

#318 Add ESLint support and configure it to use Drupal standards. #319

Conversation

hkirsman
Copy link
Contributor

@hkirsman hkirsman commented Oct 28, 2022

How to test:

  1. Get this branch
    git clone git@github.com:wunderio/drupal-project.git
    cd drupal-project
    git checkout feature/#318-Add-ESLint-support-and-configure-it-to-use-Drupal-standards
  2. Start Lando
    lando start (or lando rebuild if you've working on existing Lando project)
  3. Create example js file
    echo 'var foo="bar"' > web/modules/custom/phpunit_example/example.js
  4. Add the file to GIT
    git add web/modules/custom/phpunit_example/example.js
  5. Try to commit
    git commit
  6. Allow it to fix the script automatically and check the results

There's also an alternative PR here #320 that uses node service but it's currently proof of work. It breaks Code Quality scans, you need to configure name and e-mail in the node service.

Copy link
Member

@misterjoonas misterjoonas left a comment

Choose a reason for hiding this comment

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

Tested and works 👍 💯

.lando.yml Outdated
build_as_root:
# To be able to run ESLint with GrumPHP, we need Node.js on appserver.
- "apt-get update"
- "apt-get install -y nodejs"
Copy link
Member

@tormi tormi Oct 31, 2022

Choose a reason for hiding this comment

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

I would avoid such things in a template - they slow down the build time. Perhaps we should consider using custom PHP images instead with nodejs included?

@@ -1,3 +1,2761 @@
{
"lockfileVersion": 2
"name": "wunderio-drupal-project",
Copy link
Member

Choose a reason for hiding this comment

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

So far we've avoided adding lockfile dependencies to the repo for maintenance reasons. See #143 & #155. Should we change this?

@@ -23,6 +23,9 @@ grumphp:
# PHPUnit will fail with 0 tests.
phpunit:
testsuite: unit
eslint:
Copy link
Member

Choose a reason for hiding this comment

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

Let's consider other options as well like using Husky via node service.

@hkirsman hkirsman closed this Sep 23, 2023
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.

3 participants