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

docs(babel): update babelHelpers default description #397

Merged
merged 3 commits into from
May 21, 2020

Conversation

dannya
Copy link
Contributor

@dannya dannya commented May 17, 2020

Update babelHelpers options documentation, which was confusing (and likely incorrect): it reported that babelHelpers: 'inline' is the default, when the header of the section says that the default is 'bundled'.

Please verify that this documentation change reflects the actual behavior.

Rollup Plugin Name: {name}

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Update `babelHelpers` options documentation, which was confusing (and likely incorrect): it reported that `babelHelpers: 'inline'` is the default, when the header of the section says that the default is `'bundled'`.

Please verify that this documentation change reflects the actual behavior.
@dannya dannya changed the title Update babelHelpers option section default description docs(babelHelpers): Update babelHelpers option section default description May 17, 2020
@shellscape shellscape requested a review from Andarist May 18, 2020 18:23
@shellscape shellscape changed the title docs(babelHelpers): Update babelHelpers option section default description docs(babel): update babelHelpers default description May 18, 2020
@Andarist
Copy link
Member

The existing description is correct. It refers to "inline" being Babel's default behavior - not @rollup/plugin-babel's default behavior (which is "bundled").

@dilyanpalauzov
Copy link

Perhaps a sentence stressing explicitly that plugin-babel changes Babel's default?

@dannya
Copy link
Contributor Author

dannya commented May 19, 2020

@Andarist thanks for your clarification - I still feel as though the current text is hard to understand and not fully clear, so I've made a second attempt to improve this.

@@ -95,7 +95,7 @@ We recommend to follow those guidelines for each possible value:
- `'runtime'` - you should use it especially when building libraries with rollup. It has to be used in combination with `@babel/plugin-transform-runtime` and you should also specify `@babel/runtime` as dependency of your package (don't forget to tell rollup to treat it as your external dependency when bundling for `cjs` & `es` formats).
- `'bundled'` - you should use it if you want your resulting bundle to contain those helpers (at most one copy of each). Useful especially if you bundle an application code.
- `'external'` - use it only if you know what you are doing. It will reference helpers on **global** `babelHelpers` object. Used in combination with `@babel/plugin-external-helpers`.
- `'inline'` - this is not recommended. Helpers will be inserted in each file using them. This can cause serious code duplication (this is the default Babel behaviour).
- `'inline'` - this is not recommended. Helpers will be inserted in each file using them. This can cause serious code duplication. **(for reference, this is the default behaviour of Babel, but it is overridden by the `babelHelpers` value of this plugin: to either your own explicit selection, otherwise the default value of `'bundled'` will be used)**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `'inline'` - this is not recommended. Helpers will be inserted in each file using them. This can cause serious code duplication. **(for reference, this is the default behaviour of Babel, but it is overridden by the `babelHelpers` value of this plugin: to either your own explicit selection, otherwise the default value of `'bundled'` will be used)**
- `'inline'` - this is not recommended. Helpers will be inserted in each file using them. This can cause serious code duplication. This is the default Babel behavior as Babel operates on isolated files, as Rollup is a bundler and is project-aware the default of this plugin is `"bundled"`.

How about this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Andarist made a further amendment to your suggestion: 14fe234

@Andarist Andarist merged commit 867c699 into rollup:master May 21, 2020
@Andarist
Copy link
Member

@dannya thanks!

LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
* Update babelHelpers option section default description

Update `babelHelpers` options documentation, which was confusing (and likely incorrect): it reported that `babelHelpers: 'inline'` is the default, when the header of the section says that the default is `'bundled'`.

Please verify that this documentation change reflects the actual behavior.

* docs(babel): based on PR feedback, make a second attempt at clarifying the babelHelpers default description.

* docs(babel): based on PR feedback, make a third attempt at clarifying the babelHelpers default description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants