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

more explained error code for name-less umd bundle #3393

Merged
merged 3 commits into from Mar 6, 2020

Conversation

rail44
Copy link
Contributor

@rail44 rail44 commented Feb 20, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

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:

Description

refs #3372

In above PR, I was suggested to tweak error code for UMD bundle.
Because this patch will be breaking change for people already using this code in plugins, I create PR based on release-2.0.0 .

Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Not in favor of this change. It's still too vague in the larger context of Rollup, and if we're going to make a breaking change then a clear, concise, explanatory error code should be chosen.

@lukastaegert
Copy link
Member

@shellscape What would be your suggestion? For me, the important part is that it should be aligned with the same error code for IIFE which is added in #3372. The error is that the name option was not used even though the bundle has exports, so MISSING_NAME_FOR_EXPORT is not too far off, considering there is also the prose error message. These codes are mainly to enable filtering errors and warnings without relying on matching error messages. We could extend it slightly to MISSING_NAME_OPTION_FOR_EXPORT or MISSING_NAME_OPTION_FOR_IIFE_EXPORT to emphasize that this is solely about the IIFE part of the UMD output that does not work. Opinions?

@lukastaegert
Copy link
Member

I have changed the error code as suggested by me and also further extended the error message to explain why we need a name at all (and we have an IIFE error for UMD bundles). @shellscape would this work for you so that we can proceed here?

…these messages as of course it makes sense CircleCI does not expose the token for different repo PRs for security reasons. Babel is using an AWS Lambda here, maybe something like that.
@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #3393 into release-2.0.0 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-2.0.0   #3393   +/-   ##
=============================================
  Coverage             95%     95%           
=============================================
  Files                171     171           
  Lines               5801    5801           
  Branches            1712    1712           
=============================================
  Hits                5511    5511           
  Misses               157     157           
  Partials             133     133
Impacted Files Coverage Δ
src/finalisers/umd.ts 94.91% <ø> (ø) ⬆️

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 3f1052b...6585b7f. Read the comment docs.

@lukastaegert lukastaegert merged commit a792bb1 into rollup:release-2.0.0 Mar 6, 2020
lukastaegert added a commit that referenced this pull request Mar 6, 2020
* more explained error code for name-less umd bundle

* Align error code with IIFE warning

* Fix missing auth token issue. I guess we need to rethink how we post these messages as of course it makes sense CircleCI does not expose the token for different repo PRs for security reasons. Babel is using an AWS Lambda here, maybe something like that.

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
@lukastaegert lukastaegert mentioned this pull request Mar 6, 2020
9 tasks
lukastaegert added a commit that referenced this pull request Mar 6, 2020
* more explained error code for name-less umd bundle

* Align error code with IIFE warning

* Fix missing auth token issue. I guess we need to rethink how we post these messages as of course it makes sense CircleCI does not expose the token for different repo PRs for security reasons. Babel is using an AWS Lambda here, maybe something like that.

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
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

3 participants