as-ideas / oil

191 stars 56 forks source link

fix: Only request vendorlist once #209

Closed Fumler closed 6 years ago

Fumler commented 6 years ago

Fixes #206

This PR sets a flag whenever you do a request for the vendorlist, so if there is several calls to loadVendorList() before the request is resolved and cache is saved, we wait for the first request to resolve in the other calls.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 779


Files with Coverage Reduction New Missed Lines %
dist/oil.bundle.js 32 27.38%
<!-- Total: 32 -->
Totals Coverage Status
Change from base Build 732: 0.7%
Covered Lines: 1129
Relevant Lines: 3360

💛 - Coveralls
Fumler commented 6 years ago

Does not seem like your test coverage works properly? The coverage report basically say everything in oil.bundle.js is red, so the 27% coverage is just variables etc?

I don't know how to fix this. Maybe one of you guys know where to start? 😅

phogel commented 6 years ago

To be honest, I am not sure how exactly coveralls gets to this result. I'd say a decrease of "0.06%" is neglectable.

Fumler commented 6 years ago

Think I overcomplicated things a bit, the new commit makes it even simpler, I didn't think about fetchJsonData being a promise, so we can just return that.

And about the coveralls thing, it seems to me like nothing really works? In the screenshot below you can see everything is red in loadVendorlist even though you have a million tests for it that should cover those lines. So the 0.06% is just because I add a few new lines to the bundle but the new test I added is not reflected in the coverage?

screen shot 2018-09-07 at 14 21 59
Fumler commented 6 years ago

Any updates on this?

Fumler commented 6 years ago

Sorry for nagging, but we really want this released. Does anyone have time to CR and merge this or should we start using a fork?

@ltparis2018 @tbtz @phogel

marcelb commented 6 years ago

@Fumler Heya, I'm back in the team and will look into this today!

marcelb commented 6 years ago

Thanks! This is merged and we will create a new release as well soon.