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

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

Closed
torbjorn-boost opened this issue Oct 3, 2017 · 8 comments
Closed

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

torbjorn-boost opened this issue Oct 3, 2017 · 8 comments

Comments

@torbjorn-boost
Copy link

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
Copy link
Owner

lukeed commented Oct 3, 2017

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!

@lukeed lukeed closed this as completed Oct 3, 2017
@gaearon
Copy link

gaearon commented Jan 13, 2018

FYI, we're starting the work to compile deps with babel-preset-env in Create React App: facebook/create-react-app#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
Copy link
Owner

lukeed commented Jan 14, 2018

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
Copy link
Owner

lukeed commented Jan 14, 2018

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
Copy link

gaearon commented Jan 14, 2018

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
Copy link
Owner

lukeed commented Jan 14, 2018

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

@mrchief
Copy link

mrchief commented Aug 17, 2018

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
Copy link
Owner

lukeed commented Aug 27, 2018

@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

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

4 participants