Conversation
src/index.js
Outdated
| } | ||
|
|
||
| let result; | ||
| if (query.lazy) { |
There was a problem hiding this comment.
Maybe rename lazy => async, bc of a proposed sync option (#50) and therefore for consistency? And it is semver major anyways :)
There was a problem hiding this comment.
- Lazy references a common naming convention for
loadingof something. - Async is about order of execution or lack there of to be more specific.
Within this particular context i think lazy is the correct name for the query option. Given a bundle is either loader lazy or not I don't see the need to define a sync option given it's the only other practical alternative to lazy and the default if you don't explicitly lazy load the bundle.
There was a problem hiding this comment.
If there will never be a sync option, because it doesn't make sense then leaving lazy is better yep :).
src/index.js
Outdated
| import loaderUtils from 'loader-utils'; | ||
|
|
||
| export function pitch(remainingRequest) { // eslint-disable-line no-unused-vars | ||
| const query = loaderUtils.getOptions(this) || {}; |
There was a problem hiding this comment.
nitpick 😛 query => options
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
======================================
Coverage ? 0%
======================================
Files ? 2
Lines ? 14
Branches ? 4
======================================
Hits ? 0
Misses ? 11
Partials ? 3
Continue to review full report at Codecov.
|
| { | ||
| "useBuiltIns": true, | ||
| "targets": { | ||
| "node": "4.3" |
| ' }\n', | ||
| `}${chunkNameParam});`]; | ||
| } | ||
| return result.join(''); |
There was a problem hiding this comment.
join('\n') and remove the \n foreach item? Can be follow up style PR though
|
We need to be careful with what is exported here aswell as the main part is the named export for the pitching loader |
|
I was actually just looking at that. iirc etwp is the same way? no? |
|
Any way it goes, i'm not merging it. I'll push a beta directly from this branch. I'm not big on majors without a supporting test suite |
|
src/cjs.js const loader = require('./index');
module.exports = loader.default;
module.exports.pitch = loader.pitch;? Not 💯 :) |
|
I'm 90% sure you are right about that |
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
========================================
Coverage ? 0.00%
========================================
Files ? 2
Lines ? 14
Branches ? 4
========================================
Hits ? 0
Misses ? 11
Partials ? 3 Continue to review full report at Codecov.
|
2 participants
Intended to be merged & released as a part of
1.0.0on a betadist-tagonce this has been finished and properly tested.BREAKING CHANGE:
Enforces NodeJS > 4.3 via engines
Closes #43