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

Adding to cache as soon as require.cache is called instead of pending… #129

Merged
merged 2 commits into from
May 5, 2017

Conversation

rorticus
Copy link
Contributor

… it for later, issue #124

Type: bug

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

It looks like maybe there was some logic to defer a require.cache operation until it was required for the first time, so it could get a reference module, but what's being done now doesn't really line up with that. It's now adding to the cache as soon as you call require.cache.

Resolves #124

@codecov
Copy link

codecov bot commented Apr 28, 2017

Codecov Report

Merging #129 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   85.63%   85.63%   -0.01%     
==========================================
  Files           1        1              
  Lines         550      543       -7     
  Branches      137      136       -1     
==========================================
- Hits          471      465       -6     
  Misses         32       32              
+ Partials       47       46       -1
Impacted Files Coverage Δ
src/loader.ts 85.63% <100%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 020f647...0cc8292. Read the comment docs.

@dylans dylans added this to the 2017.05 milestone Apr 29, 2017
Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

There are lines of uncovered code here that we should really address.

@rorticus
Copy link
Contributor Author

rorticus commented May 5, 2017

@kitsonk Is that in relation to these cache changes, or just an overall observation? If its just an in-general thing, we should raise another issue for it and get them cleaned up 👍

@kitsonk
Copy link
Member

kitsonk commented May 5, 2017

@rorticus it is relation to this PR: Hits 471 464 -7 Because the codecov.io doesn't have the remapping configuration, I can't easily see which lines it is. But some code touched in this PR (more lines) is going in untested.

@rorticus rorticus merged commit d6f18f8 into dojo:master May 5, 2017
@rorticus rorticus deleted the issue-124 branch May 5, 2017 17:24
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