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

make local file extension optional #5

Merged
merged 3 commits into from
May 21, 2018
Merged

Conversation

jstrimpel
Copy link

When the localesToKeep option has at least one element the plugin is not filtering out any locales when running webpack 3.x.x and moment 2.9.x and code splitting using CommonsChunkPlugin. This PR makes the .js file extension optional and fixes filtering with the above versions. This hasn't been tested using other version combinations.

@iamakulov
Copy link
Owner

iamakulov commented Feb 22, 2018

Wow, that’s interesting.

Mind sharing your webpack.config.js + exact webpack & Moment.js version?

(And thanks for the PR!)

@jstrimpel
Copy link
Author

jstrimpel commented Feb 22, 2018

webpack@3.10.0
moment@2.9.1

I replace some content with .... common is CSS loaders and some other plugins. locales is an array of valid locales (I removed the invalid ones after the plugin threw). Thanks.

module.exports = merge(common, {
  entry: {
    core: [path.join(ARCHETYPE_ROOT, 'conf', 'webpack',  'application', '.css.js')].concat(entry),
    chrome: ['chrome'],
    main: [...]
  },
  resolve: {
    alias: Object.assign({}, alias.components, alias.core)
  },
  output: {
    filename: '[name].min.js',
    path: path.join(MODULE_ROOT, target)
  },
  devtool: 'source-map',
  module: {
    rules: [
      {
        test: /\.js$/,
        include: (() => {
          const paths = [path.resolve(MODULE_ROOT, 'src')];

          return reduce(alias.components, (acc, cPath) => {
            acc.push(path.dirname(cPath));
            return acc;
          }, paths);
        })(),
        use: {
          loader: require.resolve('babel-loader'),
          options: resolveBabelPlugins(babel)
        }
      }
    ]
  },
  plugins: [
    new MomentLocalesPlugin({
      localesToKeep: locales
    }),
    new CopyWebpackPlugin([{
      ...
    }, {
      ...
    }]),
    new UglifyJSPlugin({
      sourceMap: true
    }),
    new webpack.DefinePlugin({
      'process.env': {
        'NODE_ENV': JSON.stringify('production')
      }
    }),
    new webpack.optimize.CommonsChunkPlugin({
      name: 'common',
      filename: 'common.min.js',
      minChunks: 2,
      chunks: ['chrome', 'main']
    }),
    new webpack.optimize.CommonsChunkPlugin({
      name: 'core',
      minChunks: Infinity,
      filename: 'core.min.js'
    })
  ]
});

@iamakulov
Copy link
Owner

Thanks!

And what’s the exact webpack & Moment.js version? (Just to test this myself)

@jstrimpel
Copy link
Author

webpack@3.10.0
moment@2.9.1

@iamakulov
Copy link
Owner

Great! I’ll take a look into this in a day or two.

@iamakulov iamakulov self-assigned this Feb 22, 2018
@iamakulov
Copy link
Owner

iamakulov commented Mar 24, 2018

Great! I’ll take a look into this in a day or two.

Huh, this took a bit longer :D

I was unable to reproduce this with moment@2.9.0 (there’s no 2.9.1), webpack@3.10.0 and moment-locales-webpack-plugin@1.0.5. Here’s the config I tried with:

const MomentLocalesPlugin = require('moment-locales-webpack-plugin');

module.exports = {
    entry: {
        main: './src/index.js',
    },
    output: {
        path: __dirname,
        filename: '[name].bundle.js',
    },
    plugins: [
        new MomentLocalesPlugin({
            localesToKeep: ['ru'],
        }),
    ],
};

I also tried adding the CommonsChunkPlugin, but that had no effect.

Is this issue still reproducible for you?

@jstrimpel
Copy link
Author

Not sure. I ended up doing a copy and paste + modifying as work around, so I have my own local copy with a reference to this issue. I am currently traveling, but I will check if it is still occurring upon return.

@iamakulov
Copy link
Owner

iamakulov commented Mar 24, 2018

Thanks! 👍

@iamakulov iamakulov mentioned this pull request May 16, 2018
@iamakulov iamakulov merged commit 9b35782 into iamakulov:master May 21, 2018
@iamakulov
Copy link
Owner

iamakulov commented May 21, 2018

Merged as there’s #8 with the same issue. Thank you for the PR!

@iamakulov iamakulov mentioned this pull request May 23, 2018
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.

2 participants