lukeed / obj-str

A tiny (96B) library for serializing Object values to Strings.
MIT License
249 stars 7 forks source link

create-react-app won't build when depending on obj-str #2

Closed torbjorn-boost closed 7 years ago

torbjorn-boost commented 7 years ago

This is a weird one, and I bet this isn't the right place for it. Running yarn build in my create-react-app project, it fails with:

$ react-scripts build
Creating an optimized production build...
Failed to compile.

Failed to minify the code from this file:

        ./node_modules/obj-str/dist/obj-str.es.js:2 

Read more here: http://bit.ly/2tRViJ9

error Command failed with exit code 1.

Following the link, it recommends filing an issue, even though I don't see why this isn't working.

My dependencies are:

"axios": "^0.16.2",
"date-fns": "^1.28.5",
"immutability-helper": "^2.4.0",
"lodash": "^4.17.4",
"normalize.css": "^7.0.0",
"obj-str": "^1.0.0",
"react": "^16.0.0",
"react-dom": "^16.0.0",
"react-redux": "^5.0.6",
"react-scripts": "1.0.14",
"redux": "^3.7.2",
"redux-devtools-extension": "^2.13.2",
"redux-thunk": "^2.2.0",
"reselect": "^3.0.1"
lukeed commented 7 years ago

Hey there,

Unfortunately, that's because of this issue.

This module exposes a module entry in the package.json, which webpack will read by default. However, CRA doesn't respond to this behavior, so the export default function()... statement is making it into your build, which then makes Uglify throw, then prints the message you received.

Closing since this is a heavily discussed CRA issue, but you could manually delete the module field on the node_module/obj-str/package.json file and you should be fine. Annoying, but it works.

FWIW: CRA should be adding a include function that only includes/compiles node_modules if they have a "module" entry. This is what we did in preact-cli for a while.

Hope that helps!

gaearon commented 6 years ago

FYI, we're starting the work to compile deps with babel-preset-env in Create React App: https://github.com/facebookincubator/create-react-app/pull/3776

Let us know if you have feedback about how this should work.

That said, I disagree with the conclusion in this thread. The module entry only signals whether the project uses ES Modules—it isn't really related to other ES6 features (and won't help with a similar issue as new versions of ES come out).

lukeed commented 6 years ago

Hey @gaearon, I saw that -- cool~!

And correct, it's just a hand-off. I'm not sure if it was Rollup specifically, but the working "spec" stated that a module-entry should be ES5 code but with a ESM imports and exports. Again, it's purely a hand-off to make everyone else's lives easier.

The (main?) reason a lot of browser modules are taking advantage of the "module" field is that it allows for better scope-hoisting inside the final bundle. This was especially true of Webpack — not sure if it's still the case, but importing a CJS module added 40-50 bytes to interop.

... I highly doubt this is still the case for Webpack, but it is still the case with rollup-plugin-commonjs.

Lastly, to help out Windows users specifically, I'd look at using an include() function for that PR. You can traverse each node_module package and assert against the existence of a "module" and/or "js:next" key. Then you know which resolver to use.

It's slightly more work at the start, but uses less resources while watching.

lukeed commented 6 years ago

I realize that's probably still not enough to sway your position, haha, but I've never been in a position to influence that kind of decision. Instead, I've always had to accomodate the trends.

Supporting a bunch of different keys is definitely a little more awkward, for now, but we're also in an awkward transitional stage.

gaearon commented 6 years ago

Sorry, I got confused by this thread—just realized that in this particular case the build was failing precisely because of ESM and not because of some other ES6 feature (which is usually the case). I agree the module field makes sense as a singal for ESM entry point.

lukeed commented 6 years ago

Haha no problem. Not entirely familiar with CRA, sorry to say, so I was also taking a stab at the likely culprit.

mrchief commented 6 years ago

I'm using CRA that is 1 week old, so I'm pretty sure I'm on a very latest version of it. I still get this error. Without asking me to read those 1000 comments (I did try that too, but it says somewhere it should work and then there is follow up issue which is open), do you know if there is a solution?

lukeed commented 6 years ago

@mrchief It should now be fixed 👍 I'm not sure how, but I forgot to publish v1.0.1 after producing fully transformed ES5 code (as seen in dist).

Long overdue now, apologies