preactjs / preact-cli

😺 Your next Preact PWA starts in 30 seconds.
MIT License
4.69k stars 375 forks source link

error appearing in production build #129

Closed swarajd closed 7 years ago

swarajd commented 7 years ago

This might not be an issue with preact-cli, but in case it is I'm posting it here. I am using preact-cli and cytoscape.js, cytoscape-dagre, and cytoscape-qtip. Running the dev server produces no errors, but after building without prerender "preact build "--no-prerender", I receive this error: http://imgur.com/5NzRUaI. This: is the code that is mainly attributed to cytoscape: https://gist.github.com/swarajd/e6c6d5d4f9cf8dd27d5a97816074e25b. Are there any settings I could set to create a build application that isn't heavily minified/uglified but still does not yield any errors? Performance isn't necessarily a priority for this application as long as the build works.

prateekbh commented 7 years ago

i guess next release smight fix this as I have included node modules in prerender now

developit commented 7 years ago

I think there's a fix for SourceMaps waiting to be released that would at least give you the full source + line numbers so it'd be easier to debug, too.

swarajd commented 7 years ago

Here is the error more clearly: http://imgur.com/a/UDce9. It's a part of the cytoscape code and I think I might know what the issue is, but if not it remains a mystery.

swarajd commented 7 years ago

I found the specific issue: http://imgur.com/a/UDce9. An undefined value is being passed in for whatever reason, and causing the code to crash. I'm not sure if this was optimized out during build or never there in the first place but that seems to be the specific issue.

edit: I found the issue it was in the "dagre" NPM library: http://i.imgur.com/VEUuVzr.png. I guess it's time to make a pull request to them haha

developit commented 7 years ago

@swarajd There's a lot of tech here to sift through, but your screenshots got me wondering: where are you kicking off Cytoscape in your code? Curious if it's in componentWillMount or componentDidMount.

(note: I had accidentally commented on the other thread)

swarajd commented 7 years ago

@developit I'm kicking it off in componentDidMount.

image

edit: I've done a bit of searching around and I think a possible issue may be that cytoscape, cytoscape-dagre, or the dagre npm libraries don't like to be minified or don't play well when minified and thus cause these issues. Although I'm not entirely sure.

developit commented 7 years ago

hmm - what is rendering the element with id="cy"? If it's the root of that component you could try:

renderDiagram() {
  this.cy = cytoscape({
    container: this.base
    // ...
  });
}
swarajd commented 7 years ago

Yeah it's the root, and that change doesn't make a difference. It works in the dev server just fine, but only when it's minified does an issue occur. I tested this by running

preact build --no-production --no-prerender and it worked fine, but as soon as I ran preact build --no-prerender without the --no-production flag it didn't work

developit commented 7 years ago

Ah funky. --no-production seems like a decent solution for the short term, I don't think it'd be too disastrous for bundle size.

swarajd commented 7 years ago

It actually makes the difference between 6 megabytes and 700 kilobytes ahaha. The team I am working with said it was fine that it was that bulky but I am a performance junkie so I need me that perf haha.

developit commented 7 years ago

Yikes! That's fairly terrible :( Likely inline sourcemaps and HMR causing that bloat. Maybe try running the bundle through uglify-js with no custom configuration?

swarajd commented 7 years ago

Strangely almost no difference :( screen shot 2017-06-21 at 11 18 30 pm

lukeed commented 7 years ago

How about just aliasing cytoscape (and its related packages) to the pre-built files?

Until our custom config lands (soon, I would think) you will have to either 1) dig into the node_modules/preact-cli/lib/webpack-config.js and manually add the aliases; or 2) use module-alias; or 3) use the babel-version here within a .babelrc file.

swarajd commented 7 years ago

I am definitely willing to use one of those approaches. @developit do you think this solution would be fine or do you have something in mind?

edit: actually @lukeed the cytoscape-dagre plugin is causing the issue and has dagre as a dependancy. Both packages are npm modules and neither of them contain a .min.js file. What would you recommend in that case?

lukeed commented 7 years ago

I only did a quick search, but I saw that this graphing module historically (and currently) has minification issues.

Any one of those 3 options will work for you -- although, the first is the least trustworthy since a) none of your coworkers will receive those changes and b) you will lose all changes the moment you npm install or update anything.

Until our custom config (#56) gets merged, these are AFAIK the only two options you have since this (strongly) appears to be a cytoscape issue and not a preact-cli issue.

FWIW, I'd personally go with the module-alias route. 👍


Edited because I was on phone. :)

swarajd commented 7 years ago

Thanks for all your help! I'll try it out and see how it goes :)

swarajd commented 7 years ago

So I tried module alias and it doesn't seem to work for some reason.

image

And I ran the main require in index.js:

image

And I tried requiring it as such:

image

But it doesn't seem to resolve:

image

Is this just me making a mistake or is the library not compatible with the webpack dev server?

Edit: I tried just placing cytoscape.min.js into the components/ directory and just straight up requiring it, but the build error that I had previously:

image

Is still there. Should I require the minified versions of cytoscape, cytoscape-dagre, and dagre? As doing that for just cytoscape doesn't seem to work well.

edit: I think the ultimate issue is some variables being mangled, causing references to become undefined. Would your method of aliasing potentially fix this? I'm not entirely sure because this is more of a minification problem than anything. This definitely could work but I'm struggling to see how.

developit commented 7 years ago

Hmm - module-alias only works under Node. @lukeed I actually was thinking it would be nice to automatically support _moduleAliases in the package.json right in the CLI (by injecting them into the webpack config). Thoughts?

lukeed commented 7 years ago

@swarajd @developit I'm fairly sure that module-alias will work here (a bunch of the examples in Next.js use it for the same reason). However, it seems that there's a slight typo:

"@lib": "./src/lib/"
//=> "@lib": "src/lib"

The trailing /, I think, was the problem, because your module import path is concatenated to "@lib"'s value. I remove the leading ./ because wasn't needed. So, I think this is what was happening:

import cytoscape from '@lib/cytoscape.min'
//=> became: './src/lib//cytoscape.min.js

RE: your edits --- IMO this signals that they could be shipping a broken minified copy. To rule this out, you can reproduce your code in a static HTML (non-Preact) environment, along with the minified version. If that works, then we can't blame them. 😉

So if that static env works, my next step would be to go into preact-cli code & turn off UglifyJS. That would shed light on the "mangling" question.


@developit I can add this in real quick, but I don't think there's much point TBH (under the assumption that 56 has been merged).

swarajd commented 7 years ago

@lukeed I'm pretty sure it's a mangling issue since I tested this by running

preact build --no-production --no-prerender and it worked fine, but as soon as I ran preact build --no-prerender without the --no-production flag it didn't work

I'll try out the non-preact environment as well but I don't think that's the issue.

lukeed commented 7 years ago

Right, but if I understand correctly, when you swapped in cytoscape.min.js and required that directly. Am I correct in assuming that it still failed with a regular preact build?

What if you still used cytoscape.min.js directly and used preact build --no-prerender?

swarajd commented 7 years ago

Yeah even when directly requiring the file directly, it still returns the same error. The --no-prerender option is required for me as I have a file ( cytoscape-qtip.js ) that references window.

lukeed commented 7 years ago

Sorry, I'm still trying to be sure I'm following:

Regardless of preact-cli flags, it will always fail when you are importing the .min.js file directly?

swarajd commented 7 years ago

Yes, the fact that I use cytoscape.min.js vs the npm module doesn't make a difference apparently. I believe the error is in cytoscape-dagre or dagre itself. Neither of which have readily available minified files to use although I can look deeper into that.

lukeed commented 7 years ago

Basically, this what I'm trying to uncover (and just doing a bad job at communicating 😴):

If the above is correct, then this is not a mangling issue --- at least, not a mangling issue derived from preact-cli. Instead, this is strong evidence that cytoscape does not minify properly (regardless if it's our build or their shipped build).

This would also mean that it's not related to this dagre library because cytoscape is the only variant && the only instance wherein the build is functional is when cytoscape is not minified.

swarajd commented 7 years ago

Actually minified cytoscape + dev build works. I do agree with you with the fact that cytoscape has minification issues. I did do a bit of research and I found that their use of weaver causes issues when trying to minify (https://github.com/maxkfranz/weaver/issues/2) although that might not be the precise reason.

lukeed commented 7 years ago

Ah okay, thanks. There goes that theory 😆

rkostrzewski commented 7 years ago

@swarajd in the next release source maps will be enabled for production builds. Should help in debugging.

developit commented 7 years ago

This might also be related to #144 .

lukeed commented 7 years ago

Ah true, possibly

reznord commented 7 years ago

But this has to be tested !! I will do it once with all the latest code in my local repo

reznord commented 7 years ago

@lukeed @developit I will take it on and test it thoroughly. Once, all are tested properly, I will close this

developit commented 7 years ago

This should be fixed in 1.3.0, released last night!

swarajd commented 7 years ago

Is there a way to update the cli safely to version 1.3.0?

thangngoc89 commented 7 years ago

@swarajd It should be backward compatible so just update the dependencies with npm/yarn

developit commented 7 years ago

I am hoping we'll be able to add an update notifier so this becomes nice and easy. It's like any devDependency though, update away :)