-
Notifications
You must be signed in to change notification settings - Fork 66
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
Uglify errors in react scripts #73
Comments
I just ran into the exact same issue. It's definitely a bug related to the contents of the |
@stevemao could you please remove The code pointed to by The problem here is that src had ES6 code (including You can read more about it here: https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features |
It's definitely a bug related to the contents of the package.json now having "module"
How is adding modern JavaScript to `module` is a bug? We still have `main` entry which point to the same old JavaScript so its not a breaking change. On the other hand, you are using a deprecated minifier and wrong webpack config.
The code pointed to by modules needs to be ES5 code but with ES6 modules (import/export).
Who says that? Module entry is supposed to point to scripts with Es modules. Tools usually use this entry for tree shaking.
…Sent from my iPhone
On 7 Nov 2018, at 7:05 am, Jon Harris ***@***.***> wrote:
@stevemao could you please remove modules from package.json?
The problem here is that src had ES6 code (including let etc).
You can read more about it here: https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
You can read more about it here: https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features
`let` is not future JavaScript. It became standard 3 years ago. If I published typescript, stage 3 features/syntax or flow then its a bug and should be transpiled. If you need to support old environment, you can use Babel env to transpile modern JavaScript to old version.
…Sent from my iPhone
On 7 Nov 2018, at 7:05 am, Jon Harris ***@***.***> wrote:
@stevemao could you please remove modules from package.json?
The code pointed to by modules needs to be ES5 code but with ES6 modules (import/export).
The problem here is that src had ES6 code (including let etc).
You can read more about it here: https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Did you read the link I posted? Basically the authors of webpack & rollup ALL say that
I'm using the latest version of webpack and the latest minifier that it supports. I maintain several projects that use webpack, so I hardly think I'm using a wrong webpack config. It worked fine until the version of autobind-decorator that broke the rules.
That is not how the spec for modules in package.json modules works. If you don't want to believe that then I guess people will need to find alternatives if they want to use autobind decorators with modern tools like webpack or rollup. Have you tried bundling the latest of autobind-decorator with either rollup or webpack? I suggest you try... |
modules should point to ES5 code with ONLY es6 modules.
I don’t necessarily agree. That article was published a long time ago when es6 was new (the future javascript). Some people only want to target modern browsers (with full support of modern JavaScript), are they forced to run es5 code only?
Have you tried bundling the latest of autobind-decorator with either rollup or webpack?
I’m using webpack and I even target ie11 because of the business requirements. I always use Babel to transpile node modules from modern to targeted javascript env (this is what create react app is doing).
…Sent from my iPhone
On 7 Nov 2018, at 8:29 am, Jon Harris ***@***.***> wrote:
Who says that? Module entry is supposed to point to scripts with Es modules. Tools usually use this entry for tree shaking.
Did you read the link I posted? Basically the authors of webpack & rollup ALL say that And since they are the ones who added it to package.json and implemented tree shaking they're probably the best authority on the subject.
On the other hand, you are using a deprecated minifier and wrong webpack config.
I'm using the latest version of webpack and the latest minifier that it supports. I maintain several projects that use webpack, so I hardly think I'm using a wrong webpack config. It worked fine until the version of autobind-decorator that broke the rules.
If you need to support old environment, you can use Babel env to transpile modern JavaScript to old version.
That is not how the spec for modules in package.json modules works. If you don't want to believe that then I guess this package has hit a dead end and people will need to find alternatives if they want to use autobind decorators with modern tools like webpack or rollup.
Have you tried bundling the latest of autobind-decorator with either rollup or webpack? I suggest you try...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I guess a year and a half is a long time in JS land. lol For what it's worth, Dan Abromov (author of create react app) also said the same thing (a year ago): webpack/webpack#1979 (comment) If you want to deviate from the original intention of the I guess I'll leave it here and let other users of this package chime in if they feel the need to. In any case, speaking of "modern javascript" the new "class properties" proposal & babel-plugin let you just replace:
with:
This is a simpler alternative for our projects than having to compile all source in node_modules to be able to use new versions of this package... |
I have seen many packages exporting modern javascript via the
That is already mentioned in the README and I think it's a good alternative |
Related Bug: |
@stevemao interesting, can you share some examples of packages that are exporting es6 syntax (other than es6 modules) via the |
In any case, we only added a |
lol, except you can't use minification anymore with webpack without transpiling node modules first. The right way to fix it is to use separate lib/es outputs like most other libraries do. Happy to offer pointers if you're open to it. It's just a matter of forking your babel config to change the modules parameter to either preset-es2015 or preset-env... A good example is here: https://github.com/react-bootstrap/react-bootstrap/blob/master/.babelrc.js |
@jharris4 They use jsx in the src so they have to compile it. For those who want to use modern javascript and only target modern browsers/env what's your recommendation? |
@stevemao, For now, we need to look at a workaround. The default create-react-app / react scripts does not support this code. And most devs use the create-react-app. Hence a lot of people who have autobind-decorator dependency will run into this error soon. I personally wasted a day before I tracked this issue to this library. My recommendation is to create a 3.x version with ES6 features and maintain a 2.x release for ES5 until react start supporting ES6 by default. What do you think? |
It does. I'm using create-react-app too. I think we can add a note on README. |
I have added a note just now on readme. let me know if it makes sense |
ya, I wasted a couple of hours tracking this down myself. Definitely should have been a version bump as this is definitely a breaking change. As a workaround, I've just switched to class properties like I posted earlier. so I'm not using this package anymore... @stevemao I don't have a good answer for how to allow npm packages to be consumed in es6+ only mode. As far as I know nobody has really solved this problem yet, but maybe a new |
If we pointed Thanks @jharris4 . I personally periodically improve my build script and look up create-react-app as a good reference. They have removed deprecated uglify-js and they also compile node_modules (but only from standard javascript). |
I couldn't build my app built on Create-React-App using React DnD. Thanks @harinair for pointing to the related thread. If others are experiencing this, the workaround with react dnd is to pin autobind-decorator to version 2.1.0. Since then, I'm able to build just fine. |
@shouvik44 did you read the blog I linked? |
@stevemao I read through it, but I didn't understand how to take action. I can't stop using react-DnD, and the older version of autobind-decorator works well for my current needs. |
LOL. |
And that means that they do it wrong. |
Man, you don't get it: bundlers do always prefer the |
OBVIOUSLY, introduce a new |
You don't get it, introducing |
@catamphetamine If I have to argue with you I'll repeat what I have said. Your arguments haven't been constructive and convincing but rather mocking and attempting to taunt me. I suggest be patient and read the babel blog again rather than believing in what you think is right. Coz it's not. |
@catamphetamine If you want to learn how to discuss with people properly in a constructive way, have a look at #78 by @lroling8350. That issue so far has provided really good value. And I really appreciate everyone's inputs. Peace. |
I am running into an Uglify error that was traced to autobind-decorator@2.2.1
autobind-decorator@2.1.0 works fine. The following is the error:
This error is happening in a "let" statement. From what I read I feel Uglify cannot handle ES6 code and create react scripts does not convert all node modules to ES5 before feeding to Uglify. My assumption is some build option is changed from 2.1.0 to 2.2.1 so that the build is provided in ES6 not pure ES5.
The text was updated successfully, but these errors were encountered: