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
Clearer empty chunk warning #3244
Clearer empty chunk warning #3244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3244 +/- ##
==========================================
+ Coverage 91.27% 91.31% +0.03%
==========================================
Files 170 170
Lines 5926 5930 +4
Branches 1795 1797 +2
==========================================
+ Hits 5409 5415 +6
+ Misses 311 310 -1
+ Partials 206 205 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to add a CLI test that verifies the output (there should be some to copy from). Otherwise, really nice, thanks!
Yes, please! The CLI is the main place where we are missing coverage. Admittedly, you will have to work a little around the color escape codes as they are system dependent, so the tests need to use matchers to check the output. |
Agreed. |
@lukastaegert added tests for this error. If you'd like tests for the rest too I need to spend some time trying to figure out how to trigger them. Wondering if unit tests for |
True, but this would require a new setup, but more importantly, unit tests are nice for development but usually much harder to maintain as they resist refactoring. At the moment, Rollup has >90% coverage with basically only end-to-end testing and this is amazing with regards to maintainability as it allows for far-reaching refactorings with a great level of security with regard to breakage.
That would definitely exceed this ticket. If you want to go for it, awesome, but this should not block the current issue from being resolved. Could also be done in a technical PR separately. You would learn a lot about possible Rollup errors in the process, though 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great, thanks a lot!
if (!allWarnings.has(warning.code as string)) allWarnings.set(warning.code as string, []); | ||
(allWarnings.get(warning.code as string) as RollupWarning[]).push(warning); | ||
if (!allWarnings.has(warning.code!)) allWarnings.set(warning.code!, []); | ||
(allWarnings.get(warning.code!) as RollupWarning[]).push(warning); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
stderr: stderr => { | ||
assert.ok(stderr.includes('(!) Generated empty chunks\na, b')); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers: #3243
Description
Currently if an empty chunk is generated the warning text reads:
which is slightly confusing.
Now it will refer to the chunk and include the chunk name instead (see #3243 (comment))
I also updated
batchWarnings
to batch together multiple empty chunk messages, and tested this locally:I can't see any automated tests for this. Should I add some?
I left the
code
asEMPTY_BUNDLE
, because I guess it would be a breaking change to update that too.