Check return value of define factory before setting module.exports #26
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 82 89 +7
Branches 23 27 +4
=====================================
+ Hits 82 89 +7
Continue to review full report at Codecov.
|
| exports.hey = stuff.boi; | ||
| })(require, exports, module) | ||
| `, | ||
| `_${AMD_DEFINE_RESULT}` |
There was a problem hiding this comment.
This test depends on how babel internally generates unique variable names, which isn't great, but can't really think of another way to test this other than doing some funky regex stuff that just doesn't seem worth it.
|
@captbaritone just wondering if you saw this? No worries if you don't have time to review, just let me know and I'll seek an alternative :) |
|
Oh! I did see it, but then I put it on a todo list and promptly forgot about that list. I'll try to take a look later today. |
|
Didn't get to it this weekend, sorry. |
|
No worries :) |
|
I think it's time I admitted that I'm probably not going to get to this. Sorry! |
|
No problem, going to do a bit more testing and merge, and next release will be 1.0.0 so not worried about it breaking anything for existing users :) |
msrose
force-pushed
the
define-result-check
branch
from
bb68a11 to
dbd1aa8
Compare
This PR takes care of the behaviour described at the end of the readme on master.
There's no pressing reason to address this; I just want to do it for complete correctness.
I remember seeing webpack taking care of AMD by doing a check like this on the return value of the factory function, so that offers a bit of validation on the strategy used.
I've chosen to use babel's utilities for generating unique variables in scope for creating the temporary variable used. The other option is wrapping everything in another IIFE but since that adds up to one extra function call per modue at runtime, I figured generating a unique variable at compile time is a better option.
It's possible to use this pattern for every single transformation made, but doing it on a need-only basis makes updating the test suite easier. Also the output is a bit more understandable without the extra code in there, which makes debugging easier.
@captbaritone I'd appreciate your review for this. I'd also like to release a 1.0.0 after this gets merged - is there anything else you think we should take care of before doing that?