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

fix: unecessary program updates by removing timeout methods #1693

Merged
merged 10 commits into from Mar 12, 2020

Conversation

sheetalkamat
Copy link
Contributor

This should fix unnecessary program updates that happen right away whenever something changes

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @sheetalkamat!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@bradzacher
Copy link
Member

bradzacher commented Mar 7, 2020

Thanks for looking into the performance for us! I really appreciate the help 😄

It looks like this change breaks the persistent parse tests, which ensure that the IDE use case is working as expected.

you can run them locally from within the typescript-estree package:

cd packages/typescript-estree
yarn test persistentParse

@bradzacher bradzacher added the bug Something isn't working label Mar 7, 2020
@sheetalkamat
Copy link
Contributor Author

cd packages/typescript-estree
yarn test persistentParse

Will do

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #1693 into master will decrease coverage by 0.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #1693      +/-   ##
==========================================
- Coverage   95.21%   95.19%   -0.02%     
==========================================
  Files         148      148              
  Lines        6953     6967      +14     
  Branches     2006     2007       +1     
==========================================
+ Hits         6620     6632      +12     
- Misses        123      124       +1     
- Partials      210      211       +1
Impacted Files Coverage Δ
...pt-estree/src/create-program/createWatchProgram.ts 87.02% <81.81%> (-0.11%) ⬇️

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 10, 2020
@sheetalkamat sheetalkamat dismissed a stale review via d9a5867 March 11, 2020 00:01
@sheetalkamat
Copy link
Contributor Author

@uniqueiniquity This PR is ready to review and merge with the changes we talked about

@sheetalkamat
Copy link
Contributor Author

And I am not sure failing codecov/patch is about

@uniqueiniquity
Copy link
Contributor

@bradzacher any thoughts on the code coverage issue? it's a preemptive change for 3.9 so it makes sense that the path isn't taken.

@bradzacher
Copy link
Member

bradzacher commented Mar 11, 2020

If you want to satisfy the branch coverage, you should be able to use istanbul ignore comments to stop the coverage reporting the missed branch.

/* istanbul ignore if */ if (isRunningNoTimeoutFix) {
// ....
}

/* istanbul ignore else */ if (!isRunningNoTimeoutFix) {
// ....
}

I keep it around to try and prompt contributors to cover their code appropriately, though I don't usually treat code coverage as a hard merge blocker because there's always some edge cases and checks to keep the types "safe" (unless of course the coverage is terrible, then I RC 😅).

When it fails, I usually review the coverage report to see what code was missed, and if it's something that should be tested.
(yellow = if has a missed branch, red = code not executed)

If you're happy with this change, approve it and I'll gladly override the failed check to merge it.

(again, thanks to you both for looking into all of this for us)

@sheetalkamat
Copy link
Contributor Author

I think we should ignore the coverage result instead of adding ignore comment since as soon as 3.9 releases things should start working... I will update this with master for merge

@bradzacher bradzacher merged commit 2ccd66b into typescript-eslint:master Mar 12, 2020
@sheetalkamat sheetalkamat deleted the noTimeouts branch March 12, 2020 19:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants