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

Group files by config #622

Merged
merged 9 commits into from Oct 27, 2021
Merged

Conversation

Spence-S
Copy link
Contributor

@Spence-S Spence-S commented Oct 19, 2021

based on work in #621

Fixes #599

options = overrides.options;

if (overrides.hash) {
xoConfigPath += overrides.hash;
Copy link
Contributor

@fisker fisker Oct 20, 2021

Choose a reason for hiding this comment

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

Let's add a new variable, maybe xoConfigHash?

Copy link
Contributor Author

@Spence-S Spence-S Oct 20, 2021

Choose a reason for hiding this comment

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

I changed to eslintConfigId to make it clear we need unique id for each unique eslint config.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 20, 2021

@sindresorhus sindresorhus changed the title fix: group files by config (closes #599) Group files by config Oct 20, 2021
@Spence-S
Copy link
Contributor Author

Spence-S commented Oct 20, 2021

I'm still getting the issue I mentioned in #621 (comment):

sindresorhus/ow/runs/3947275882?check_suite_focus=true#step:5:8

Note that that project explicitly specifies a tsconfig: sindresorhus/ow@f1c83bb/package.json#L80-L82

I was able to fix this, I tried to add a failing test that would cover this case, but I couldn't figure out how to get it to fail, even when I tried to emulate the configs from ow 🤷🏻 - there is some mysteries in @typescript-eslint to me. However, the latest change seems to fix the issue correctly now.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 20, 2021

I tried the latest commit here and it's now back to running out of memory: https://github.com/sindresorhus/ow/runs/3955265790

@Spence-S
Copy link
Contributor Author

Spence-S commented Oct 20, 2021

ahh I see - so I think maybe the regression is not exactly what we thought then...
We now cache a different tsconfig for each file separately. In v .43 we cached the minimum amount of tsconfigs as needed. I think this is pretty easily fixable - I will do some performance testing before the next fix.

@Spence-S
Copy link
Contributor Author

Spence-S commented Oct 20, 2021

@sindresorhus looking more deeply into this - I have a question. Do you know why we cache the tsconfig files at all?

I see the need for when there is no tsconfig present. In that case it makes sense to define and cache it for linting. - However, If we can find the tsconfig in the users dir - it makes no sense to me why we cache any more 🤷🏻 - I don't think the normalization is needed, but I could be wrong.

It only slows performance otherwise as far as I can tell.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 21, 2021

I'm not sure to be honest. This was implemented a long time ago and not by me. I agree with your reasoning though.

@Spence-S
Copy link
Contributor Author

Spence-S commented Oct 21, 2021

I changed the logic to only cache when no tsconfig can be found. - All tests were passing for me locally, but I think the failing test is minor. I changed a couple of tests regarding the tsconfig caching logic and added a few more.

When linting ow with no cache, it went from 45s run time to 14s locally for me and now only requires 1 call to eslint.

@sindresorhus
Copy link
Member

sindresorhus commented Oct 21, 2021

@Spence-S Great. It now seem to work well and doesn't run out of memory: https://github.com/sindresorhus/ow/runs/3966300112?check_suite_focus=true

@Spence-S
Copy link
Contributor Author

Spence-S commented Oct 21, 2021

Bizarre why those tests started failing. I don't think I changed anything about them - just a refactor to make them much easier to read and split the assertions into their own test funcs got them passing again. This should be able to merged after a review now.

@sindresorhus sindresorhus merged commit 431887d into xojs:main Oct 27, 2021
3 checks passed
@sindresorhus
Copy link
Member

sindresorhus commented Oct 27, 2021

Thanks for contributing this! 👏

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