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

Disable errors for duplicate emitted file names #3175

Merged
merged 2 commits into from Oct 20, 2019

Conversation

marijnh
Copy link
Contributor

@marijnh marijnh commented Oct 18, 2019

And add a test to make sure such emits no longer error spuriously.

Issue #3174

This PR contains:

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

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

List any relevant issue numbers:

#3174

Description

This works around an issue produced by a race condition in the plugin driver when multiple outputs are generated concurrently. Different outputs emitting the same file name via emitFile would fail. @lukastaegert indicated he's planning a proper fix for this, but in the meantime, this unblocks ezolenko/rollup-plugin-typescript2#180 (comment) by just turning off the error that was being reported incorrectly.

@marijnh marijnh force-pushed the no-duplicate-filename-error branch 2 times, most recently from 3100b4e to 3c9ee28 Compare Oct 18, 2019
@marijnh
Copy link
Contributor Author

marijnh commented Oct 18, 2019

There were two tests checking for the presence of this error. I've updated the patch to skip them.

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #3175 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3175      +/-   ##
==========================================
- Coverage    90.2%   90.19%   -0.01%     
==========================================
  Files         165      165              
  Lines        5910     5907       -3     
  Branches     1800     1799       -1     
==========================================
- Hits         5331     5328       -3     
  Misses        350      350              
  Partials      229      229
Impacted Files Coverage Δ
src/utils/FileEmitter.ts 100% <ø> (ø) ⬆️
src/utils/error.ts 100% <0%> (ø) ⬆️

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 f99c7e4...d9421ef. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Looks good. Just some minor comments.

@@ -50,7 +49,8 @@ function generateAssetFileName(

function reserveFileNameInBundle(fileName: string, bundle: OutputBundleWithPlaceholders) {
if (fileName in bundle) {
return error(errFileNameConflict(fileName));
// FIXME this should return error(errFileNameConflict(fileName));
// but until #3174 is fixed, this raises spurious errors and is disabled
Copy link
Member

@lukastaegert lukastaegert Oct 20, 2019

Choose a reason for hiding this comment

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

From my side, I would eventually go for a warning instead of an error.

@@ -1,4 +1,5 @@
module.exports = {
skip: true,
Copy link
Member

@lukastaegert lukastaegert Oct 20, 2019

Choose a reason for hiding this comment

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

I would actually just delete the tests altogether. Otherwise there is a high danger that the tests rot, and we could restore them from git anyway. If we switch to a warning, the tests need to be rewritten anyway.

@marijnh marijnh force-pushed the no-duplicate-filename-error branch from 69043e4 to c8d4dbd Compare Oct 20, 2019
@marijnh
Copy link
Contributor Author

marijnh commented Oct 20, 2019

Updated the patch to remove those tests entirely (I set them to skip since it is my experience that it's easy to forget restoring removed tests).

I'm not sure what do do about your comment about the comment content.

marijnh and others added 2 commits Oct 20, 2019
And add a test to make sure such emits no longer error spuriously.

Issue rollup#3174
* Add test description
* Describe better in the TODO comment what the ultimate intention is
@lukastaegert lukastaegert force-pushed the no-duplicate-filename-error branch from c8d4dbd to d9421ef Compare Oct 20, 2019
@lukastaegert
Copy link
Member

lukastaegert commented Oct 20, 2019

Thanks!

I set them to skip since it is my experience that it's easy to forget restoring removed tests

Personally I would just write new tests, especially since the combinatorics are not too complicated, the new tests should test for a warning instead of an error and tests that do not run as part of the full pipeline quickly become forgotten and meaningless.

@lukastaegert
Copy link
Member

lukastaegert commented Oct 20, 2019

I'm not sure what do do about your comment about the comment content.

No problem, I adjusted the comment slightly.

@lukastaegert lukastaegert merged commit 06fb16a into rollup:master Oct 20, 2019
6 of 7 checks passed
@marijnh
Copy link
Contributor Author

marijnh commented Oct 21, 2019

Thank you!

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