zaceno / hyperapp-partial

Helps you structure your hyperapp code
MIT License
7 stars 0 forks source link

Error when building with webpack/uglify plugin #1

Closed RikuVan closed 7 years ago

RikuVan commented 7 years ago

Thanks for this cool experiment. I ran into a problem doing a production build with webpack (you may well question why am I using this in production already). Anyways, any idea what it could be?

ERROR in bundle.js from UglifyJs
Invalid assignment [./node_modules/hyperapp-partial/dist/index.js:1,745][bundle.js:59544,742]
RikuVan commented 7 years ago

By the way, a typo in the docs. The version of this in the docs is missing something parentheses (added here):


const myPartial = partial.mixin('myPartial', emit => ({
    state: {
      propX: ...,
      propY: ...,
    },
    actions: {
      add: ...,
      subtract: ...,
    }
}))
jorgebucaran commented 7 years ago

@RikuVan Did you compile the code to ES5?

Uglfiyjs can't mangle stuff like this.

RikuVan commented 7 years ago

I run it through babel with some presets since I am using ES6 and other stage 2 stuff in my own code. Will play with it more though when I have time.

jorgebucaran commented 7 years ago

Maybe you are missing that one specific preset needed? Not sure.

Anyways, any idea what it could be?

Looks like some ES6 code is pushing its way to uglify js.

zaceno commented 7 years ago

@RikuVan Thanks for the props, and docs-corrections. I'll see to that shortly. As for your build problems... I'm not sure, but I suspect that maybe my browserify-bundle isn't compatible with es6 imports. (which I assume you're using)

The code is actually written with lots of es6 because node.js/browserify and most modern browsers support all of that without babel. Just not the import-export syntax yet, which is why I still use CommonJS (because I want to avoid babel)

And I don't think es6 + uglifyjs is the problem, because I'm actually using uglify-es (which uglifies es6 code). You just can't tell because uglify-es is called uglifyjs on the command-line for backwards compatibility.

So... I'm going to have to look into this more. But for now, can you confirm @RikuVan, that you are importing hyperapp-partial using es6 import syntax? Or are you require ing?

zaceno commented 7 years ago

I feel fairly certain this is the problem, in package.json: "main": "dist/index.js",

I want to make partial usable via CDN at http://unpkg.com/hyperapp-partial (which is just soo convenient). They serve whatever is the main file of your bundle. So I have to use browserify with the --standalone flag to make a UMD bundle for my main file.

But now when you install the package via npm and require it, you're requiring not the original source file but the bundled file. Since it's UMD, it works well with require. But I wonder how well it works with webpack and/or babel and es6 import statements. (Neither of which I use personally, so that's probably why I didn't notice this issue)

If only unpkg.com could be made to serve something other than "main"...

I'm open to any thoughts or ideas of how I can fix this! :)

jorgebucaran commented 7 years ago

@zaceno Why don't you just copy & paste from hyperapp pkg.json? This is a "solved problem".

RikuVan commented 7 years ago

I also suspected it had to do with the fact it had already been bundled. Tried both import and require and neither worked. It works fine though in my dev build. So the only difference is when using the Uglify plugin for the prod build.

RikuVan commented 7 years ago

Its fine if I am just copy paste master, which is what I will do. No sweat for me but you probably want it to work for anyone who downloads from npm.

RikuVan commented 7 years ago

As a side-note it was interesting to notice that the actions for this mixin don't show up in the hyperapp redux dev tool. Perhaps an issue for that repo.

zaceno commented 7 years ago

@jbucaran copy and paste what? Ah you perhaps mean this:

 "jsnext:main": "src/index.js",
  "module": "src/index.js",

...remind me again, please what those do? I seem to recall from our earlier conversation, that jsnext:main is there because hyperapp uses rollup? If so that shouldn't make a difference here. "module" I don't know...

jorgebucaran commented 7 years ago

@zaceno I'd suggest you use rollup to simplify everything. If you do, it's just copy and paste from hyperapp. In other words, build -partial the same way hyperapp is already built.

zaceno commented 7 years ago

@jbucaran sure, thanks for the suggestion. But I don't feel like bringing in another tool makes things simpler. I prefer to go as bare as possible always.

If I must use another tool for this to work for others, I will of course. I just want to get to the bottom of why first

jorgebucaran commented 7 years ago

You are already using browserify, that's a tool.

jorgebucaran commented 7 years ago

Also, it seems you are using uglify-es instead of uglify-js. Maybe @RikuVan is not using uglify-es.

zaceno commented 7 years ago

Yes. but add yet another? Or oh, you're saying swap browserify for rollup? That's a bit more palatable. But still, there'd be an extra config file right?

jorgebucaran commented 7 years ago

Hyperapp doesn't use a config file 🎉

rollup -i src/index.js -o dist/hyperapp.js -m -f umd -n hyperapp \
  | uglifyjs dist/hyperapp.js -o dist/hyperapp.js \
  --mangle \
  --compress warnings=false \
  --pure-funcs=Object.defineProperty -p relative \
  --source-map dist/hyperapp.js.map
jorgebucaran commented 7 years ago

BTW rollup might give you a smaller bundle file too.

zaceno commented 7 years ago

@jbucaran yes you're probably on to something with the uglify-es6. My bundle is minified es6 code. Still, if @RikuVan is using babel, it should be able to convert my bundle to es5 and all should be well.

In that case, not sure using rollup would help here. Because hyperapp is written using es5 (except the import/export) right?

jorgebucaran commented 7 years ago

You are right! I am just using this issue to push my secret agenda! 😏

zaceno commented 7 years ago

@RikuVan

Its fine if I am just copy paste master, which is what I will do. No sweat for me but you probably want it to work for anyone who downloads from npm.

Yes, absolutely I do! So, even though you're ok with the status quo for now, would you mind helping me clarify this a bit so I can work out what's up, and what, if anything, I can do about it? 🙏

I also suspected it had to do with the fact it had already been bundled. Tried both import and require and neither worked.

Ok so it's probably not an issue with the module-system. I have require d it in my projects with no problem. If require doesn't work for you, it must be something else.

It works fine though in my dev build. So the only difference is when using the Uglify plugin for the prod build.

Ok this is interesting... And you are using babel to transpile your project from es6 to es5, before passing through uglifyjs right?

Could it be that your toolchain skips es6->es5 conversion for stuff in node_modules ? That would result in a bundle with a mix of es6 and es5 code, and that would break uglifyjs of course.

zaceno commented 7 years ago

Ok I found this: https://babeljs.io/docs/usage/babel-register/

Specifically this:

NOTE: By default all requires to node_modules will be ignored. You can override this by passing an ignore regex via: ....

It seems the majority are still transpiling to es5, and it would be just plain mean of me to tell all of them to add a config line specifically for my module.

So the only tenable solution, it seems is for me to bite the bullet and start outputing es5-code. I'll take care of that shortly.

(and I'll probably have to use babel cause I can't remember how es5 code looks anymore 😛 )

zaceno commented 7 years ago

Ok version 0.2.0 out with es5 code in the dist bundle. Sadly that brings the lib up to 1.2kb minified and compressed. 😓 I can probably do better if I write the es5 code by hand, but that's for later.

For now: @RikuVan you wanna test this in your project and see if this works better for you?

RikuVan commented 7 years ago

Yep, now it builds :thumbsup:

zaceno commented 7 years ago

Glad to hear it! Thanks for bringing this to my attention @RikuVan