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.

Sign up for GitHub

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

Jump to bottom

Add .babelrc error to configureBabelPresetEnv and remove some deprecations #651

Merged
merged 1 commit into from Nov 23, 2019
+39 −4
Merged

Add .babelrc error to configureBabelPresetEnv and remove some deprecations #651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/WebpackConfig.js
View file
Open in desktop
Expand Up @@ -405,10 +405,6 @@ class WebpackConfig {
normalizedOptionKey = 'includeNodeModules';
}

if (['useBuiltIns', 'corejs'].includes(optionKey)) {
Copy link
Contributor

@ihmels ihmels Oct 10, 2019 •
edited

Choose a reason for hiding this comment

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

If we don't deprecate the options, it's possible to do:

Encore
  .configureBabel(null, {
    corejs: 3,
  })
  .configureBabelPresetEnv((options) => {
    options.corejs: 2
  });

I think it's better to update the flex recipe instead of not deprecating the options because the recipe recommends them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we don't deprecate the options, it's possible to do:

Encore
 .configureBabel(null, {
   corejs: 3,
 })
 .configureBabelPresetEnv((options) => {
   options.corejs: 2
 });

I don't think that's a real issue, the options have the same name and it's quite unlikely someone will try to set them in both configureBabel() and configureBabelPresetEnv() (and even more unlikely that they will with different values).

That's also the case for other methods in Encore, for instance if you call Encore.enableVueLoader(() => {}, { useJsx: true }), and Encore.configureBabel(options => { options.presets = []; }).

I think it's better to update the flex recipe instead of not deprecating the options because the recipe recommends them.

It doesn't really cost us anything to keep them here, but it will cost time to users that initialized Encore using the flex recipe if we remove them.

But as I said that's not the only reason: I think having some real options that we can document / add checks on beside callbacks is a good thing. Maybe it could be argued that in this case they should be set by configureBabelPresetEnv() but that's not a big issue either (it's still related to Babel after all).

logger.deprecation(`configureBabel: "${optionKey}" is deprecated. Please use configureBabelPresetEnv() instead.`);
}

if (this.doesBabelRcFileExist() && !allowedOptionsWithExternalConfig.includes(normalizedOptionKey)) {
logger.warning(`The "${normalizedOptionKey}" option of configureBabel() will not be used because your app already provides an external Babel configuration (a ".babelrc" file, ".babelrc.js" file or "babel" key in "package.json").`);
continue;
Expand Down Expand Up @@ -456,6 +452,10 @@ class WebpackConfig {
throw new Error('Argument 1 to configureBabelPresetEnv() must be a callback function.');
}

if (this.doesBabelRcFileExist()) {
throw new Error('The "callback" argument of configureBabelPresetEnv() will not be used because your app already provides an external Babel configuration (a ".babelrc" file, ".babelrc.js" file or "babel" key in "package.json").');
}

this.babelPresetEnvOptionsCallback = callback;
}

Expand Down
35 changes: 35 additions & 0 deletions test/WebpackConfig.js
View file
Open in desktop
Expand Up @@ -642,6 +642,41 @@ describe('WebpackConfig object', () => {
});
});

describe('configureBabelPresetEnv', () => {
beforeEach(() => {
logger.reset();
logger.quiet();
});

afterEach(() => {
logger.quiet(false);
});

it('Calling method sets it', () => {
const config = createConfig();
const testCallback = () => {};
config.configureBabelPresetEnv(testCallback);
expect(config.babelPresetEnvOptionsCallback).to.equal(testCallback);
});

it('Calling with non-callback throws an error', () => {
const config = createConfig();

expect(() => {
config.configureBabelPresetEnv('FOO');
}).to.throw('must be a callback function');
});

it('Calling with a callback when .babelrc is present throws an error', () => {
const config = createConfig();
config.runtimeConfig.babelRcFileExists = true;

expect(() => {
config.configureBabelPresetEnv(() => {});
}).to.throw('your app already provides an external Babel configuration');
});
});

describe('configureCssLoader', () => {
it('Calling method sets it', () => {
const config = createConfig();
Expand Down