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

Clearer empty chunk warning #3244

Merged
merged 7 commits into from Nov 20, 2019

Conversation

tjenkinson
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (existing tests updated. Couldn't find any test for batchWarnings. let me know if there are some)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers: #3243

Description

Currently if an empty chunk is generated the warning text reads:

Generated an empty bundle

which is slightly confusing.

Now it will refer to the chunk and include the chunk name instead (see #3243 (comment))

Generated an empty chunk: "main"

I also updated batchWarnings to batch together multiple empty chunk messages, and tested this locally:
image

I can't see any automated tests for this. Should I add some?

I left the code as EMPTY_BUNDLE, because I guess it would be a breaking change to update that too.

@tjenkinson tjenkinson changed the title Clearer empty bundle warning Clearer empty chunk warning Nov 19, 2019
@tjenkinson tjenkinson marked this pull request as ready for review November 19, 2019 08:31
@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

Merging #3244 into master will increase coverage by 0.03%.
The diff coverage is 56.25%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/Chunk.ts 93.97% <100%> (+0.01%) ⬆️
cli/run/batchWarnings.ts 29.03% <53.33%> (+3.41%) ⬆️

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 edddb12...d225a2c. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a 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!

@lukastaegert
Copy link
Member

I can't see any automated tests for this. Should I add some?

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. cli/samples/custom-frame would be an example of such a kind of test.

@lukastaegert
Copy link
Member

I left the code as EMPTY_BUNDLE, because I guess it would be a breaking change to update that too.

Agreed.

@tjenkinson
Copy link
Member Author

@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 batchWarnings might work better instead? Would be simpler to test all the different codes then

@lukastaegert
Copy link
Member

Wondering if unit tests for batchWarnings might work better instead?

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.

If you'd like tests for the rest too I need to spend some time trying to figure out how to trigger them

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 😉

Copy link
Member

@lukastaegert lukastaegert left a 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);
Copy link
Member

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'));
}
};
Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

None yet

2 participants