feat(babel): add filter option #767
Conversation
|
Thanks for the PR. Unfortunately we cannot consider this PR until tests are added for the change. Please add tests or we'll have to close. |
|
Hi @shellscape , As per this issue, rollup/rollup#3013 (comment) Do you think it's a better solution to add I found it's kind of weird to add test for |
|
It seems that maybe instead of adding more |
|
@Andarist if it makes sense for the plugin, then I'm in agreement. we'll definitely want the docs to be clear about why this plugin behaves differently than the other ones, and why it accepts different filtering options. |
chengcyber
force-pushed
the
feat-babel-resolve
branch
from
ee1715d
to
282634c
Compare
chengcyber
changed the title
|
refactor the code and add filter option to @rollup/plugin-babel instead. |
packages/babel/README.md
Outdated
| @@ -75,6 +75,21 @@ Type: `String | RegExp | Array[...String|RegExp]`<br> | |||
|
|
|||
| A [minimatch pattern](https:/isaacs/minimatch), or array of patterns, which specifies the files in the build the plugin should operate on. When relying on Babel configuration files you cannot include files already excluded there. | |||
|
|
|||
| ### `filter` | |||
|
|
|||
| Type: (id: unknown) => boolean<br> | |||
There was a problem hiding this comment.
doesn't this always receive a string?
There was a problem hiding this comment.
Originally, the type should be ReturnType<typeof createFilter>. As per https:/rollup/plugins/blob/master/packages/pluginutils/types/index.d.ts#L52
The createFilter returns (id: string | unknown) => boolean. and it was id: unknown inferred in IDE when I write the code...
Anyway, I changed the type to (id: string) => boolean. PTAL
chengcyber
force-pushed
the
feat-babel-resolve
branch
from
50e42a9
to
6cb165a
Compare
packages/babel/src/index.js
Outdated
| skipPreflightCheck, | ||
| ...babelOptions | ||
| } = unpackInputPluginOptions(pluginOptionsWithOverrides, this.meta.rollupVersion)); | ||
|
|
||
| const extensionRegExp = new RegExp( | ||
| `(${extensions.map(escapeRegExpCharacters).join('|')})$` | ||
| ); | ||
| const includeExcludeFilter = createFilter(include, exclude); | ||
| filter = (id) => extensionRegExp.test(stripQuery(id).bareId) && includeExcludeFilter(id); | ||
| const userDefinedFilter = (id) => { |
There was a problem hiding this comment.
Sorry for not mentioning that before (I would sweat that I've done that but apparently I didn't) - I would not try to compose those things here. If a custom filter gets passed in I would just throw if exclude or include would be passed in as well.
Something along those lines:
if (customFilter && (include || exclude)) {
throw new Error('...')
}
const configFilter = customFiler || createFilter(include, exclude);
filter = (id) => extensionRegExp.test(stripQuery(id).bareId) && configFilter(id);There was a problem hiding this comment.
Change the code logic, switch to your design. and add test as well.
Rollup Plugin Name:
babelThis PR contains:
Are tests included?
Breaking Changes?
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
SincecreateFilterhas a third options, which can passresolutionBase. It might be better the option can be controlled byrollup-plugin-babelAccording to the following discussion, adding filter option to @rollup/plugin-babel.