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

fix: output.format should be esm instead of es #3220

Closed
wants to merge 1 commit into from
Closed

fix: output.format should be esm instead of es #3220

wants to merge 1 commit into from

Conversation

JounQin
Copy link

@JounQin JounQin commented Nov 10, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

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

Breaking Changes?

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

List any relevant issue numbers:

Description

@codecov
Copy link

codecov bot commented Nov 10, 2019

Codecov Report

Merging #3220 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3220   +/-   ##
=======================================
  Coverage   90.64%   90.64%           
=======================================
  Files         167      167           
  Lines        5911     5911           
  Branches     1793     1793           
=======================================
  Hits         5358     5358           
  Misses        336      336           
  Partials      217      217
Impacted Files Coverage Δ
src/rollup/index.ts 93.93% <100%> (ø) ⬆️

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 6fed410...5a933f4. 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.

It is impossible to write a test because nothing was broken. If you disagree, prove it by adding a test that fails without the change. The reason is that internally for compatibility with plugins, esm is mapped to es. This will change with the 2.0 release. Please do not guess how the code works, test it. The requirement of tests in the issue template is there for a reason.

@JounQin
Copy link
Author

JounQin commented Nov 10, 2019

@lukastaegert It breaks my codes like following which was working for a long time:

-      rollup(opts).then(bundle => bundle.write(opts as OutputOptions)),
+      rollup(opts).then(bundle => bundle.write(opts.output as OutputOptions)),

I'm wondering why it was not even mentioned in CHANGLOG.

@lukastaegert
Copy link
Member

Something breaks your code, but it is not fixed by your change, and if you had tested it, you would have known. And I would have known what you were actually trying to achieve. I will push a fix for #3222, which I accidentally broke. To my defence, this was an undocumented and untested feature from before my time.

@JounQin
Copy link
Author

JounQin commented Nov 11, 2019

I would say that it did work by adding esm instead of replacing es to esm on my testing.

And the internal esm -> es mapping and the error message is very confusing for someone new to the source code like me.

Whatever, glad to hear that we're going to have a fix for it.

@JounQin JounQin deleted the patch-1 branch November 11, 2019 05:30
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