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

CI: Update ESLint Config #5722

Closed
wants to merge 3 commits into from

Conversation

loicginoux
Copy link
Contributor

Summary

the es lint upgrade changed the way eslint config file is declared. we had to rename and migrate config options to the new configuration system

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

the es lint upgrade changed the way eslint config file is declared. we had to rename and migrate config options to the new configuration system
@loicginoux loicginoux requested a review from a team as a code owner April 12, 2024 09:11
@github-actions github-actions bot added changelog:repository Changes to the repository not within any gem changelog:solidus_admin labels Apr 12, 2024
@tvdeyen
Copy link
Member

tvdeyen commented Apr 12, 2024

@loicginoux nice! Thanks. I just disabled the new config in #5721 but this is probably better because its future proof :)

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Nice, but we should not pretend to be a npm package. We just use the package.json for dependency management for ESLint (and maybe other JS tools), but never will publish Solidus as NPM package 😆

package.json Show resolved Hide resolved
@tvdeyen tvdeyen changed the title fix JS linter in CI CI: Update ESLint Config Apr 12, 2024
@tvdeyen
Copy link
Member

tvdeyen commented Apr 12, 2024

It seems there is more involved. Maybe we merge #5721 first and then continue the work here?

@loicginoux
Copy link
Contributor Author

I'll continue exploring the CI environment, I guess it's just a missing npm install in CI env.

@loicginoux loicginoux marked this pull request as draft April 12, 2024 10:41
npm install is needed to run the lint:js rake task
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.94%. Comparing base (a2e1519) to head (2f7a74e).
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5722   +/-   ##
=======================================
  Coverage   88.94%   88.94%           
=======================================
  Files         702      702           
  Lines       16698    16698           
=======================================
  Hits        14852    14852           
  Misses       1846     1846           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@loicginoux loicginoux marked this pull request as ready for review April 12, 2024 11:01
@loicginoux
Copy link
Contributor Author

It looks good now @tvdeyen, I can squash commits if requested.

@loicginoux loicginoux requested a review from tvdeyen April 12, 2024 12:47
@tvdeyen
Copy link
Member

tvdeyen commented Apr 17, 2024

I merged #5721 now. I am not sure we want this change as this means we need to maintain a package.json with its dependencies and a custom eslint config file. Before we where just using npx and the default eslint config.

@tvdeyen tvdeyen requested review from a team and removed request for tvdeyen April 17, 2024 12:09
@tvdeyen tvdeyen dismissed their stale review April 17, 2024 12:09

addressed

@loicginoux
Copy link
Contributor Author

I understand, fair point... I leave you close the issue if you think this problem is solved.

@tvdeyen
Copy link
Member

tvdeyen commented Apr 18, 2024

@solidusio/core-team wdyt?

@kennyadsl
Copy link
Member

I like the completeness of this solution, but considering that ESlint is a very very small part of this project, I'd avoid any maintenance effort for the plugin. I'd probably rather remove it and try to use something else. I think that for now the solution @tvdeyen proposed is enough, but I'd keep this as a reference if we want to change idea later.

Thanks, @loicginoux and @tvdeyen !

@tvdeyen
Copy link
Member

tvdeyen commented May 3, 2024

Thanks for working on this @loicginoux but i agree with @kennyadsl and am closing this for now. Again, thanks for investing and contributing.

@tvdeyen tvdeyen closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:repository Changes to the repository not within any gem changelog:solidus_admin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants