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(perf): add cache for fs calls when using language service #1049

Merged
merged 2 commits into from
Sep 9, 2019
Merged

fix(perf): add cache for fs calls when using language service #1049

merged 2 commits into from
Sep 9, 2019

Conversation

topaxi
Copy link
Contributor

@topaxi topaxi commented Apr 1, 2019

Supersedes #940

@topaxi
Copy link
Contributor Author

topaxi commented Apr 1, 2019

lodash.memoize is a devDependency, should I include it in the dependencies? Or do we favor the old PR over this?

@coveralls
Copy link

coveralls commented Apr 1, 2019

Pull Request Test Coverage Report for Build 3533

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 90.038%

Totals Coverage Status
Change from base Build 3532: 0.005%
Covered Lines: 1069
Relevant Lines: 1129

💛 - Coveralls

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 12, 2019

I think if lodash.memoize is needed for published version then we need to add it to dependencies.

I think I don't have any questions about this. Just you need to check why the CI failed.

Cc @GeeWee @kulshekhar

@GeeWee
Copy link
Collaborator

GeeWee commented Apr 12, 2019

I agree. I didn't see it was a devDependency.
If the CI passes we should merge this in.

@kulshekhar
Copy link
Owner

One test seems to be failing on linux with node 6. Once that's fixed, this looks good to me

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 16, 2019

snapshot failed on node 6 looks strange, even though new snapshot looks identical to old snapshot. Would you please check @topaxi

@topaxi
Copy link
Contributor Author

topaxi commented Apr 17, 2019

I'm unable to reproduce this on my machine using node v6.16.0.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 17, 2019

hmm, the similar error occurs to #1056 which upgrades upgrade jest to 24.7.1, snapshot also failed even though they were identical. any thoughts @GeeWee @kulshekhar ?

@GeeWee
Copy link
Collaborator

GeeWee commented Apr 23, 2019

That's.. weird. Seems like Node 6 is running through the files in a different order for node 6 here, not identical as in #1056 - I'm thinking maybe we should drop the Node 6 build - latest LTS is node 8 anyways.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 23, 2019

I think we should. Not sure if jest already dropped support. We need to check if jest dropped then we will do too.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 27, 2019

what should we deal with node 6 failed build here @GeeWee ?

@GeeWee
Copy link
Collaborator

GeeWee commented Apr 29, 2019

I think we should disable it but only @kulshekhar has CI access I think

@Mordred
Copy link

Mordred commented May 16, 2019

I have tried this patch and it will break "watch" mode, because file changes are not reloaded.

@topaxi
Copy link
Contributor Author

topaxi commented May 16, 2019

I haven't checked further implications in this PR, I continued the work from #940. Let me know if there is anything left I can do, although this PR isn't very high in my priorities. If anyone else is up for this, feel free to take over or close 👍

@ahnpnl
Copy link
Collaborator

ahnpnl commented Sep 8, 2019

If rebasing this branch, CI no longer runs for node 6

@topaxi
Copy link
Contributor Author

topaxi commented Sep 9, 2019

Rebased

@kulshekhar
Copy link
Owner

thanks @topaxi @ahnpnl

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.

7 participants