ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.69k stars 243 forks source link

DllPlugin: the .jsx files are they bundled? #169

Closed enten closed 7 years ago

enten commented 8 years ago

File: tools/development/ensureVendorDLLExists.js

I think there is a mistake when the modules list is computed.

Indeed, jsx extension is allowed by the tools/webpack/configFactory.

But, the getJsFilesFromSrcDir function (in ensureVendorDLLExists.js) catch **/*.js files only.

function getJsFilesFromSrcDir(srcPath) {
  return globSync(`${pathResolve(appRootPath, 'src', srcPath)}/**/*.js`);
}

How about the .jsx files? I think they are skipped.

ctrlplusb commented 8 years ago

Good catch. I don't use the .jsx format myself so this is an oversight. :)

ctrlplusb commented 8 years ago

Appreciate the detailed reports!

ctrlplusb commented 8 years ago

Would you be interested in becoming a collaborator on this project and doing a PR to fix this? :)

enten commented 8 years ago

I suggest:

function getJsFilesFromSrcDir(srcPath) {
  return ['js', 'jsx']
    .reduce((acc, ext) =>
      acc.concat(globSync(`${pathResolve(appRootPath, 'src', srcPath)}/**/*.${ext}`), []);
}

So, in buildVendorDLL function, we don't need to check files extension anymore (glob.sync already do the job): - const isJsFile = file => pathExtName(file) === '.js'; - const allJSFiles = [...clientFiles, ...universalFiles].filter(isJsFile); + const allJSFiles = [...clientFiles, ...universalFiles];

Would you be interested in becoming a collaborator on this project and doing a PR to fix this? :)

If I can make real contributions, why not :)

EDIT1: initialize the reducer with an empty array EDIT2: remove useless filtering

ctrlplusb commented 8 years ago

Done! That looks great. I like your style!

ctrlplusb commented 7 years ago

On next branch.