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

Catch invalid options.format values #2813

Merged
merged 2 commits into from Apr 19, 2019
Merged

Catch invalid options.format values #2813

merged 2 commits into from Apr 19, 2019

Conversation

marijnh
Copy link
Contributor

@marijnh marijnh commented Apr 18, 2019

Before, it'd nebulously crash when trying to dereference RESERVED_NAMES_BY_FORMAT instead.

(It'd be nice if the formats were centrally defined, but since they aren't I decided to just add another list.)

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

@marijnh
Copy link
Contributor Author

marijnh commented Apr 18, 2019

Okay, yeah, see my PR #2814 wrt to the failing CI run.

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.

Looks technically ok to me except that internally, 'esm' is actually mapped to 'es' so you should check for that instead (which is why so many tests are breaking). The reason is that this is what was used by plugins before we changed to 'esm' .

The error message may be misleading but it correctly lists the currently recommended alias 'esm' (though there is a chance we will switch to 'module' in the future)

Before, it'd nebulously crash when trying to dereference
RESERVED_NAMES_BY_FORMAT instead.

(It'd be nice if the formats were centrally defined, but
since they aren't I decided to just add another list.)
@marijnh
Copy link
Contributor Author

marijnh commented Apr 19, 2019

I can't get the tests to pass either way (a lot of them time out) locally, so I've force-pushed just to see what the CI makes of it.

@lukastaegert
Copy link
Member

Seems that CI likes it!

@lukastaegert lukastaegert merged commit 099444d into rollup:master Apr 19, 2019
@@ -34,7 +34,7 @@ function checkOutputOptions(options: OutputOptions) {
});
}

if (!options.format) {
if (['amd', 'cjs', 'system', 'es', 'iife', 'umd'].indexOf(options.format) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

You forgot esm

Copy link
Member

Choose a reason for hiding this comment

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

Never mind. I saw Lukas comment

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