Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

Fix Webpack build for published packages, puny refactor #1289

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Dr1Ku
Copy link

@Dr1Ku Dr1Ku commented Jun 22, 2017

First up, thanks for an excellent starter-kit, using this in prod for some time now, keep up the great work!

I've ran into an issue with using packages based on the starter kit.

If you're basing a project upon the starter kit and you publish it to, say, a privately hosted NPM registry, compilation may fail if the path up to the project,

say /home/proj/starter-kit,
includes node_modules, e.g. /home/proj/node_modules--this-breaks-it/starter-kit.

This is the case with an NPM registry, where running an npm i my-project-based-on-this-starter-kit automatically creates a folder named, how else, node_modules and installs the package therein. Thus, the package, when npm-installed under a path such as <wherever>/node_modules(.*?)/my-project-based-on-this-starter-kit cannot be compiled using Webpack.

It basically does not include the much needed Webpack babel-loader which should compile JSX to parseable/readable JS, yielding a stack-trace such as

Module parse failed: [..] Unexpected token
You may need an appropriate loader to handle this file type.
[...]
SyntaxError: Unexpected token [..] at [..] [(..]/node_modules/acorn/dist/acorn.js)

Narrowing it down to this was, of course, painful. A compile would work on the git checkout and fail on the deployed/installed package. An issue with CreateReactApp finally got me on the right path.

Popular opinion from Webpack's creator also semi-demonizes the use of exclude. I wanted to try include: project.basePath but that did not work. /cc @davezuko Any ideas here?

If going forward with the include fix, one would have to expand the readme and mention the fact that only appDir sources are compiled and one has to explicitly specify other paths if one needs to compile other non-src sources.

Here's a quick repro term session from my env (Win32):

react-redux-starter-kit--node_modules-in-path-webpack-fails

Or try this in a term:

# Assuming working directory is a git clone of the starter kit
λ mkdir node_modules--matches
λ cd ..
λ mv react-redux-starter-kit\ node_modules--matches\
λ cd node_modules--matches\react-redux-starter-kit--clean\
λ npm run compile

=>
clean\src\main.js Unexpected token (19:4)
You may need an appropriate loader to handle this file type.
|
|   ReactDOM.render(
|     <App store={store} routes={routes} />,
|     MOUNT_NODE
|   )
 @ multi ./src/main webpack-hot-middleware/client.js?path=/__webpack_hmr
× Compiler encountered errors. Error: Webpack compiler encountered errors

P.S.: Sorry for missing the "No capitalization on first letter" on the commit/contributing styleguide and mis-labeling the refactor-ish commit.

Meta – Related issues, which helped in narrowing the root cause, would be:

Dr1Ku added 3 commits June 22, 2017 14:30
- Running the `build` (npm) script would fail in env `production`
…oject

- Fixes an issue where the path up to the project (e.g. `/home/proj/node_modules/rrsk`)
  would break the Webpack build
- Useful when packaging (e.g. npm) a project based on the starter kit
@dvdzkwsk
Copy link
Owner

Crazy, I never considered that the project would be run from inside of node_modules. Thanks for tracking this down, it's clear that it wasn't the easiest of tasks. Let me try to reproduce this locally. Your solution seems fine, but I agree that using include as a whitelist may make more sense; let me see if I can manage any success.

I think one potential problem with this is that it will (I think) process global node_modules with webpack. This is kind of an anti-pattern anyways, having global dependencies that is, but something I think we need to be conscious of.

No worries about the commits and whatnot, I always clean them up before merging anyways.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants