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

When.js failing test #92

Merged
merged 2 commits into from
Dec 3, 2018
Merged

When.js failing test #92

merged 2 commits into from
Dec 3, 2018

Conversation

guybedford
Copy link
Contributor

When defines itself using the wrapper pattern:

(function(define) { 'use strict';
define(function (require) {

	var timed = require('./lib/decorators/timed');
	var array = require('./lib/decorators/array');
	var flow = require('./lib/decorators/flow');
	var fold = require('./lib/decorators/fold');
	var inspect = require('./lib/decorators/inspect');
});
})(typeof define === 'function' && define.amd ? define : function (factory) { module.exports = factory(require); });

Similarly to #87 we have the static analysis problem of not knowing that the internal requires are external requires... (although in theory this static analysis is a trivial compiler problem :P).

This seems to be the bug behind "jugglingdb" builds as far as I can tell.

@guybedford guybedford mentioned this pull request Dec 3, 2018
@guybedford
Copy link
Contributor Author

One suggested fix for the above would be to "statically detect" these AMD-style wrappers, and "unwrap" them in a loader before passing back to Webpack for analysis.

@rauchg
Copy link
Member

rauchg commented Dec 3, 2018

I was just about to create a jugglingdb failing test, but it's this! :)

@guybedford
Copy link
Contributor Author

I've added the fix for this case, based on a very specific wrapper detection.

While the code is highly overly-specific, I think there will be ways to generalize the approach to better analysis methods as it expands, but i think as a starting point starting specific then moving to more general analysis makes sense, driven by the test cases.

@codecov-io
Copy link

codecov-io commented Dec 3, 2018

Codecov Report

Merging #92 into master will increase coverage by 1.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   69.78%   70.84%   +1.05%     
==========================================
  Files           3        3              
  Lines         331      343      +12     
==========================================
+ Hits          231      243      +12     
  Misses        100      100
Impacted Files Coverage Δ
src/asset-relocator.js 92.68% <100%> (+0.45%) ⬆️

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 da037bd...40b6eaa. Read the comment docs.

@rauchg
Copy link
Member

rauchg commented Dec 3, 2018

@guybedford can you:

  • fix the conflict
  • write up an issue with the more general solution description
    ?

@guybedford
Copy link
Contributor Author

Posted #95 which would replace a lot of the logic with a partial evaluator.

@rauchg rauchg merged commit c0c7a01 into master Dec 3, 2018
@rauchg rauchg deleted the failing-test2 branch December 3, 2018 21:43
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