Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Does not work with UglifyJS version 2 #938

Closed
lightninglu10 opened this issue Jul 28, 2017 · 41 comments
Closed

Does not work with UglifyJS version 2 #938

lightninglu10 opened this issue Jul 28, 2017 · 41 comments
Assignees
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up

Comments

@lightninglu10
Copy link

lightninglu10 commented Jul 28, 2017

  • Version: 0.25.0
  • Platform: JS
  • Subsystem:

Type: Bug

Severity: Critical

Description:

Trying to build IPFS node in the browser, uglifyJS through webpack won't build the app.

static/js/main.725ac8bb.js from UglifyJs
Unexpected token: name (CID) [./~/cids/src/index.js:23,0][static/js/main.725ac8bb.js:8442,6]

Hey guys, I'm trying to build an IPFS node in my browser. Building the app with yarn run build using create-react-app gives the error above. It looks like it doesn't like the type CID. I've installed CID as well, but no go. Anyone know what's going on here?

@lightninglu10 lightninglu10 changed the title Unexpected token: name (CID) [./~/cids/src/index.js:23,0][static/js/main.725ac8bb.js:8442,6] UglifyJS can't build: Unexpected token: name (CID) Jul 28, 2017
@daviddias
Copy link
Member

Thanks for reporting this, @lightninglu10 🌟

I believe this issue is related to libp2p/js-libp2p#65. tl;dr; We use the constructor name for the type checks and since Uglify changes it per module, a lack of treeshaking makes it create different names for the same module.

Weird to me that we actually do not use the constructor name anywhere in CID, I wonder if you are using the latest version of all the modules. @lightninglu10 mind trying using js-ipfs by installing it from the repo with npm install ipfs/js-ipfs? Let me know the result.

@daviddias daviddias added kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up labels Jul 29, 2017
@lightninglu10
Copy link
Author

lightninglu10 commented Jul 29, 2017

@diasdavid Installed via yarn add ipfs/js-ipfs and it still gives me the error. I setup a bare bones repo with IPFS so you can try it here. In the root just run yarn run build and you'll get the webpack error.

https://github.com/lightninglu10/ipfs-test

@daviddias
Copy link
Member

@lightninglu10 yeah, the issue comes from uglify or what uglify tries to do. We have to change the way we do some of the duck typing.

@daviddias daviddias added exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue labels Sep 1, 2017
@daviddias daviddias added the status/ready Ready to be worked label Sep 13, 2017
This was referenced Nov 8, 2017
@cristiano-belloni
Copy link

Could it be that you're not transpiling ES6? create-react-app notoriously won't compile ES6 unless you eject (which removes all the advantages of using create-react-app)

@daviddias
Copy link
Member

daviddias commented Nov 21, 2017

@daviddias daviddias changed the title UglifyJS can't build: Unexpected token: name (CID) UglifyJS Nov 21, 2017
@cristiano-belloni
Copy link

@diasdavid Unfortunately, you're essentially saying that create-react-app users won't be able to use js-ipfs. And create-react-app, on its end, won't support es6 uglifying. It's sad.

@alexsicart
Copy link

alexsicart commented Dec 2, 2017

@lightninglu10 I had the same problem with my project (build with Angular 4), when I tried to go with production mode. Let me know if you were using angular, I can help with the Uglify problem.

@thisconnect
Copy link
Contributor

You will likely run into issues with other deps using create-react-app. Either eject and fix webpack yourself. Alternativelly it should be possible to do a custom postinstall pre build of all your deps, using rollup, browserify or webpack to commonjs and import from this manual build.

@jeroenptrs
Copy link

@thisconnect @cristiano-belloni what worked for me was adding it from unpkg in index.html and then calling it in my component via const node = new window.Ipfs();

@daviddias daviddias changed the title UglifyJS Does not work with UglifyJS version 2 Feb 21, 2018
@daviddias
Copy link
Member

@sohkai
Copy link

sohkai commented Feb 21, 2018

The latest create-react-app doesn't have the most recent uglify-js, since they're currently waiting until a bigger 2.0 release to include it :(.

@fazo96
Copy link

fazo96 commented Feb 21, 2018

@diasdavid this is with create-react-app@1.1.1

Failed to compile.

Failed to minify the code from this file: 

        /home/fazo/Projects/chlu-ipfs-support/node_modules/cids/src/index.js:23 

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

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

We are currently working around the issue using this trick while we wait for create-react-app to use uglify-es or for this to land in a release

@daviddias
Copy link
Member

@fazo96 thanks so much for checking and providing those links. That trick should do it for the other folks while create-react-app doesn't get a new release. 👍 ❤️

@daviddias
Copy link
Member

daviddias commented Mar 17, 2018

The real issue is our "poor duck typing" used in some modules by checking the constructor name. @pgte's proposal at #1131 (comment) is sound and should be the adopted one.

@fsdiogo this would be a great contribution :)

@daviddias daviddias added P1 High: Likely tackled by core team if no one steps up and removed P2 Medium: Good to have, but can wait until someone steps up labels Mar 21, 2018
@daviddias
Copy link
Member

I've started the release party!

multiformats and libp2p done \o/, next up is IPLD and then IPFS

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 6, 2018

@diasdavid js-libp2p-webrtc-star and js-libp2p-utp didn't get released, only merged to master, so they haven't been updated in the libp2p dependencies.

@daviddias
Copy link
Member

@fsdiogo understood. releasing webrtc-star. utp won't be necessary as it isn't used anywhere.

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 6, 2018

@diasdavid awesome, thanks 👍 libp2p only got merged too, can you address that as well? 😇

@daviddias
Copy link
Member

daviddias commented Apr 9, 2018

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 9, 2018

@diasdavid great, thanks! The dependencies of the following repos should be updated too:

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 12, 2018

I'm having some problems with the way npm hoists the js-ipfs dependencies.

When testing locally everything works fine because when npm linking some repos, the latest version gets used, but that's not the case when I clone, update the package.json deps to the latest version and then npm i. The ipld-dag-pb that gets used is an older version, so many tests fail.

Expect some PRs updating the deps, I hope I can circumvent the problem that way.

I'll list here what needs to be merged and released:

@daviddias
Copy link
Member

done :)

@daviddias
Copy link
Member

@fsdiogo can we get a confirmation from your example that everything is working as expected and also a PR to ipfs/aegir to bring back uglify and reduce the bundle size?

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 16, 2018

@diasdavid Not quite, right now I'm using the browser-script-tag example to test the bundled ipfs versions, but I'm getting this error creating a node, even in the non-uglified bundle:

err

Going to look into it.

EDIT: a new version of stable was breaking ipld-dag-pb - ipld/js-ipld-dag-pb#66

--

About the create-react-app with IPFS build errors, this work won't fix those out of the box, as we're not serving a transpiled version of js-ipfs. The current version of CRA uses UglifyJS that still doesn't support ES6. A new version of CRA is being worked on, using uglify-es that supports ES6. Until then, devs will have to eject and change the webpacks configs to first transpile the ipfs modules, or maybe import a transpiled and bundled version of IPFS with a script tag (even thought it's quite sad to have to do that these days).

I think we should open a new issue in AEgir to serve a transpiled version of IPFS that can be used with CRA and like-minded tools, and start closing and pointing issues like this one to that issue that has all of this info. What do you guys think?

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 16, 2018

Opened a PR enabling the uglify mangling and compress.

@fazo96
Copy link

fazo96 commented Apr 19, 2018

@fsdiogo the issue with Uncaught TypeError: sort is not a function is due to Two-Screen/stable#9 and it appears only through webpack builds.

I encountered the problem in an unrelated context but had success with forcing that package to version 0.1.6 instead of the new 0.1.7 which introduced this bug

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 19, 2018

Hey @fazo96, you're right 👍, although a bit too late as I've already submitted a PR (ipld/js-ipld-dag-pb#66) a few days ago with that solution an has been merged and released, so it should be good now!

But thanks for pointing that out 🙂

@mitra42
Copy link

mitra42 commented Apr 20, 2018

I've lost track of what this is incorporated in, Does this mean we can build the compact (production) version of an app that embeds IPFS with webpack now? And if so, will that work with ipfs 0.28.2 or will it have to wait for the next release?

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 20, 2018

Hey @mitra42, this issue is pretty confusing and it's mixing multiple problems.

  • The minified version of IPFS (when you run npm run build and outputs to dist/index.min.js) wasn't being uglified (compressed and mangled) due to the way we were doing some type checks. I've made a bunch of PRs fixing this, and the final one in AEgir, enabling compress and mangle. I'm waiting for this one to be approved and merged, then we have to update the AEgir dependency in IPFS and you're good to go by bumping its version.

  • You should be able use a compact version of an app with webpack, but you have to do the webpack configs by yourself. You can check this example, although it uses an older version of webpack. I'm not very experienced with webpack, but I can try to help you if you have issues.

  • The other issue that was being discussed here was using IPFS with create-react-app. The current version of CRA uses UglifyJS that still doesn't support ES6. As IPFS doesn't offer a transpiled version, trying to build a production version of a CRA with IPFS will fail to minify. A new version of CRA is being worked on, using uglify-es that supports ES6. Until then, devs will have to eject and change the webpacks configs to first transpile the ipfs modules, or maybe import a transpiled and bundled version of IPFS with a script tag (even thought it's quite sad to have to do that these days).

@mitra42
Copy link

mitra42 commented Apr 20, 2018

Thanks @fsdiogo - I think I'm looking at the first issue (not being able to minify due to type checks).

We are using "require ipfs" to include IPFS in the larger app, and already have it working with webpack in non-minified way. I think the (example)[https://github.com/ipfs/js-ipfs/tree/master/examples/browser-webpack] is overcomplex for that, most of the webpack config seems to be to get the HTML/React examples working rather than IPFS itself. The only config we needed was the usual stuff to exclude node modules though its interesting to see you've exclude "net" and "tls" as well as the usual "fs".

So if I understand correct I have to wait till js-ipfs 0.28.3 which should have your type-checking fixes in it.

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 20, 2018

It won't be a patch, it'll be a minor release, due to my commit 5b2cf8c. But yes, you'll then be able to uglify it and not have errors due to the type checks, just be sure to use uglify-es and not uglify-js, due to the ES6 syntax from the IPFS modules.

@mitra42
Copy link

mitra42 commented Apr 20, 2018

Minor release meaning 0.28.3 ?

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 21, 2018

No, that would be a patch. Minor release is 0.29.0. IPFS follows semantic versioning.

@daviddias
Copy link
Member

I've started the issue for the next release -- #1320 --, I hope it to be ready this week and released by early next :)

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 23, 2018

Let us continue this discussion in #1321.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/expert Having worked on the specific codebase is important help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging a pull request may close this issue.