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

Update to eslint.config.js #890

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Update to eslint.config.js #890

wants to merge 17 commits into from

Conversation

glass-ships
Copy link
Collaborator

@glass-ships glass-ships commented Nov 11, 2024

eslint has migrated from the .eslintrc.json to the new "flat config", eslint.config.js style.

This PR:

  • updates to the new config style
  • addresses a number of linting/formatting/typing errors that were (for some reason) caught with the new version but not previously
  • updates some dependencies and make necessary usage changes

Manually included action changes to fix #935 from #937

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for monarch-app ready!

Name Link
🔨 Latest commit 16803e6
🔍 Latest deploy log https://app.netlify.com/sites/monarch-app/deploys/67854516a970f00008047b2f
😎 Deploy Preview https://deploy-preview-890--monarch-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.29%. Comparing base (da511ba) to head (16803e6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   71.26%   71.29%   +0.03%     
==========================================
  Files          91       91              
  Lines        3149     3149              
==========================================
+ Hits         2244     2245       +1     
+ Misses        905      904       -1     

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

@glass-ships
Copy link
Collaborator Author

@vincerubinetti Hope you're doing well, long time no see!
Sorry to drag you back, feel free to ignore.

But do you have any idea why axe.test.ts would be throwing type issues as a result of this PR?
I've tried digging around a bit but playwright is an..interesting package and it's not super clear to me what, if anything, has changed here.

@vincerubinetti
Copy link
Contributor

vincerubinetti commented Jan 9, 2025

The issue seems to be with this line:

type Test = Parameters<typeof test>[1];

I assume I wrote it like this originally because Playwright did not conveniently export a (e.g.) TestFunction type we could use to define the shape of the async ({ page, browserName }) => { function, so I had to extract it manually from the test() function like that.

My guess is that in upgrading the version of @playwright/test, somewhere along the line they added a function overload to the test() function. Now it can be:

test("some name", () => {});
// OR
test("some name", { someTestDetail: "abc" }, () => {});

which made Parameters<typeof test>[1]; read the wrong parameter number, i.e. the middle arg of the second overload there, TestDetails.

I've found that this works (for now), where these types are just imported from playwright, and this union is defined under-the-hood in node_modules/playwright/types/test.d.ts:

/** test func args */
type TestArgs = PlaywrightTestArgs &
  PlaywrightTestOptions &
  PlaywrightWorkerArgs &
  PlaywrightWorkerOptions;
// it'd be nice if they had a type for this union and exported it for us so we could just use it forever

/** generic page axe test */
const checkPage =
  (path: string, selector?: string) =>
  async ({ page, browserName }: TestArgs) => {

Or a better long term solution is to refactor it into this:

const checkPage = (title: string, path: string, selector?: string) =>
  test(title, async ({ page, browserName }) => {})

checkPage("Accessibility check " + path, path);

@glass-ships
Copy link
Collaborator Author

Nice, that last refactor seems to have done it!

Thanks so much for taking a look, I'm not sure I'd have ever figured that out haha

@glass-ships glass-ships changed the title [Experimental] Update to eslint.config.js Update to eslint.config.js Jan 10, 2025
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.

Pytest seems to be failing in certain environments
2 participants