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

Unexpected Babel configuration behavior #36

Closed
d3dc opened this issue Nov 22, 2018 · 7 comments
Closed

Unexpected Babel configuration behavior #36

d3dc opened this issue Nov 22, 2018 · 7 comments

Comments

@d3dc
Copy link

d3dc commented Nov 22, 2018

The babel config override will override the loaderOptions, presets, and plugins for both of the babel-loader rules CRA has.

It seems like the second loader shouldn't receive the presets and plugins by default, since the second loader is meant for:

// Process any JS outside of the app with Babel.
// Unlike the application JS, we only compile the standard ES features.

I would argue the behavior is unexpected since only files in src/ are supposed to get babel-preset-react-app and previously tools such as react-app-rewired only modified the first loader

@patricklafrance
Copy link
Contributor

patricklafrance commented Nov 22, 2018

Hi @d3dc

Thanks for your feedback.

Interesting, I haven't notice that.

May I ask what side effects you are experiencing since craco updates both babel loaders?

Do you see any use cases were someone would require to apply presets or plugins to both babel loaders?

Thank you

@d3dc
Copy link
Author

d3dc commented Nov 22, 2018

The second loader is intended to polyfill code for browsers with react-app-polyfills.

I don't think there should be too many side-effects. The biggest is performance. Each plugin will traverse the AST for each import - and this happens for all of node_modules with the second loader (by default the first one has exclude: /node_modules/ and its overwritten here)

IMO, the second loader is the "polyfill loader". I could see a use case where someone would want to apply a babel preset to all code to support an "environment". You might want to use the same "dependency" terminology.

Edit: I'm not sure why I confused the loaders... It really is setup to compile with all the plugins, except the helper option is passed to @babel/plugin-transform-runtime in a separate entry point. I'd have some digging in to do to really tell whether I'm wrong 🤥.

 

My use case is a little out of the ordinary - I'm trying to package my whole toolkit with a customized craco config and uncompiled library code. When using lerna's bootstrap, my babel config applies differently than when using the packaged code, because the symlink resolves. My fix is just to check which loader is being modified in my loaderOptions function and adjust include and exclude.

@d3dc
Copy link
Author

d3dc commented Nov 25, 2018

So to clarify, anything outside of src/ ordinarily only gets:

  • @babel/preset-env
  • @babel/plugin-transform-runtime
  • @babel/plugin-syntax-dynamic-import

This does seem odd to automatically add all new plugins to. But as I mentioned, you may want to add your own polyfills.

It is already pretty convenient to use a loaderOptions function:

  loaderOptions: opts => {
    if (!opts.customize) {
      // external files
      return opts
    }

    return {
      ...opts,
      ...customization
    }
  }

@patricklafrance
Copy link
Contributor

patricklafrance commented Nov 25, 2018

Hi @d3dc

This is indeed an easy fix if you want to customize what's being configured for the files outside of the src/ directory.

I might wait before updating any code for this one since I don't want to break anyone setup.

How could craco's configuration be modified to accommodate your usecase?

Maybe an applyToExternalFiles boolean option with a default value of true ? Not sure though if it wouldn't be more confusing than helping?

{
   babel: {
        presets: [],
        plugins: [],
        loaderOptions: { }
        applyToExternalFiles: true
   }
}

@d3dc
Copy link
Author

d3dc commented Dec 2, 2018

I ended up writing a custom function for my babel configuration. Its pretty close to the original, with two things:

  • it only applies settings to the "customize" loader
  • it modifies the same loader to include specific external libs
  overrideWebpackConfig({ webpackConfig, cracoConfig }) {
   ...
    const {
      externalLibs,
      presets,
      plugins,
      loaderOptions,
    } = cracoConfig.customBabel

    matches.forEach(({ loader }) => {
      if (loader.options.customize) {
        Object.assign(loader.options, loaderOptions, {
          presets: [...loader.options.presets, ...presets],
          plugins: [...loader.options.plugins, ...plugins],
        })

        if (externalLibs) {
          loader.include = (Array.isArray(loader.include)
            ? loader.include
            : [loader.include]
          ).concat(externalLibs)

          loader.exclude = {
            test: loader.exclude || /node_modules/,
            not: externalLibs,
          }
        }
      }
    })

    return webpackConfig
  }

The boolean approach is probably better for react native libraries, as there could be frequent changes to the config with my way. I would think the performance would need testing before saying a whitelist is faster, but I also think a whitelist is faster.

We both used the word "external" but I wonder if "applyOutsideSrc" is simpler.

@patricklafrance
Copy link
Contributor

patricklafrance commented Dec 2, 2018

Thanks for sharing.

I agree with you that applyOutsideSrc is more accurate.

@patricklafrance
Copy link
Contributor

Will be fix in #49

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

No branches or pull requests

2 participants