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

Failed to minify the code from this file: defs.js #89

Closed
cristiano-belloni opened this issue Sep 28, 2017 · 11 comments
Closed

Failed to minify the code from this file: defs.js #89

cristiano-belloni opened this issue Sep 28, 2017 · 11 comments

Comments

@cristiano-belloni
Copy link

This file is failing minification with react-create-app.
Their docs say:

To resolve this:
Open an issue on the dependency's issue tracker and ask that the package be published pre-compiled (retaining ES6 Modules).
Fork the package and publish a corrected version yourself.
If the dependency is small enough, copy it to your src/ folder and treat it as application code.

Did you have this kind of problem before? Why is it failing? Seems a pretty "normal" js file to me.

@cristiano-belloni
Copy link
Author

cristiano-belloni commented Sep 28, 2017

So it seems that uglifyJS, which is what create-react-app is using, doesn't support ES6 files. And, since we get fengari from github, we use the src files, that are not compiled.

I guess a unpleasant side effect of this issue, which is still unanswered - will there ever be a npm release with a consumable module? - I see it's just a matter of doing npm build on release and point the package to the dist directory.

@cristiano-belloni
Copy link
Author

It's actually a bit more complex - the target should be umd in the webpack config. I opened a PR to exemplify how I made it work: #90

@daurnimator
Copy link
Member

daurnimator commented Oct 2, 2017

Open an issue on the dependency's issue tracker and ask that the package be published pre-compiled (retaining ES6 Modules).

Definitely not. I consider build artifacts in a repository to be a huge mistake.

So it seems that uglifyJS, which is what create-react-app is using, doesn't support ES6 files.

This doesn't seem to match with your earlier statement (about retaining es6 modules in the pre-compiled file)

will there ever be a npm release with a consumable module?

Yes.

I see it's just a matter of doing npm build on release and point the package to the dist directory.

A release of this fengari repository will not include a compiled module (however end-user intended projects such as fengari-web probably will): fengari should be given to your existing bundler which may perform e.g. tree shaking.

It's actually a bit more complex - the target should be umd in the webpack config

Why? Which tool does not support commonjs style modules?

@cristiano-belloni
Copy link
Author

cristiano-belloni commented Oct 2, 2017

Definitely not. I consider build artifacts in a repository to be a huge mistake.

And I agree. But the only way to do that without an npm package is including /dist in the repo.

This doesn't seem to match with your earlier statement (about retaining es6 modules in the pre-compiled file)

Sorry about my confusing statement: What I meant is that:

  1. You can't feed ES6 code to uglifyJS - it will just die and refuse to compile at the first let statement it encounters.
  2. create-react-app uses uglifyJS by default, as you can see in the linked docs
  3. It is considered poor practice to ship code that needs to be compiled, hence the need to compile it before distributing it:

You may occasionally find a package you depend on needs compiled or ships code for a non-browser environment.
This is considered poor practice in the ecosystem and does not have an escape hatch in Create React App.

will there ever be a npm release with a consumable module?
Yes.

But until there is, there's no way to use fengari with Webpack, which is one front-end huge use case.

A release of this fengari repository will not include a compiled module (however end-user intended projects such as fengari-web probably will): fengari should be given to your existing bundler which may perform e.g. tree shaking.

Fengari can't be given to my existing bundler, because UglifyJS (which is the de facto standard in javascript uglifying) needs compiled code. Also, as in before, distributing uncompiled code is considered bad practice, at least by the Facebook and UglifyJS developers.

@daurnimator
Copy link
Member

But until there is, there's no way to use fengari with Webpack, which is one front-end huge use case.

fengari-web uses fengari with webpack.

Fengari can't be given to my existing bundler, because UglifyJS (which is the de facto standard in javascript uglifying) needs compiled code.

New uglifyJs supports es6. https://www.npmjs.com/package/uglify-es

distributing uncompiled code is considered bad practice, at least by the Facebook and UglifyJS developers.

In that case, I disagree with them.

  • There are so many compilers out there; and I have no desire for fengari to dictate which to use.
  • There are many compiler options: I don't want to pick which set for you to use
  • Not everything can be transpiled from ES6->ES5. I want to take advantage of ES6 features when available (and hence distribute as ES6, not compiled)
  • Some ES6 features require quite large polyfills for ES5 implementations. If you don't want to support old browsers, then you can have much smaller code.
  • Compiling and bundling should happen only once: at the very end of the build pipeline. This means it happens in your project, not by upstream distributing compiled modules (infact that makes it harder!)

@cristiano-belloni
Copy link
Author

cristiano-belloni commented Oct 2, 2017

fengari-web uses fengari with webpack.

AFAIK, fengari-web loads scripts directly from the DOM, while I need to programatically run scripts typed in a textarea. The only Lua code in the project would be input by the user, in other words.

In that case, I disagree with them.

Don't want to start a compiler war here, but compiling ES6 to ES5 seems to be the de-facto way of distributing a library. You are free to disagree, of course, but that would make the library more difficult to use for FE developers who use create-react-app or Webpack out of the box.

@daurnimator
Copy link
Member

daurnimator commented Oct 2, 2017

AFAIK, fengari-web loads scripts directly from the DOM, while I need to programatically run scripts typed in a textarea. The only Lua code in the project would be input by the user, in other words.

fengari-web does that by watching for script tags added to the dom and running the lua code it finds in them. https:/fengari-lua/fengari-web/blob/master/src/fengari-web.js

Instead of doing that, you could write something that runs code when user hits "submit" or such.

Don't want to start a compiler war here, but compiling ES6 to ES5 seems to be the de-facto way of distributing a library. You are free to disagree, of course, but that would make the library more difficult to use for FE developers who use react-create-app or Webpack out of the box.

I myself use webpack out of the box without issues.
I've never used react-create-app before, so don't have much to say there.

@dlbd
Copy link
Contributor

dlbd commented Oct 2, 2017

I had the same problems with boilerplate generated by vue-cli:

  • uglify failed with Unexpected token name «i», expected punc «;» [./~/fengari/src/defs.js:8,0]
  • non-uglified bundle did not work in older browsers (IE 11).

I fixed this by adding a separate Webpack loader rule for fengari (rule order may matter):

      {
        test: /.js$/,
        loader: 'babel-loader',
        include: [resolve('node_modules/fengari/src')],
        options: {
          babelrc: false,
          presets: ['env']
        }
      }

resolve is a helper function that does path.join(__dirname, '..', dir). babelrc: false was necessary for me because otherwise fengari's code was require'd through webpack/buildin/harmony-module.js at runtime, which somehow caused module.exports to be undefined.

I'm not very familiar with create-react-app, but I think you can do npm run-script eject and then edit Webpack configs manually.

@cristiano-belloni
Copy link
Author

cristiano-belloni commented Oct 2, 2017

Yep, I can eject the create-react-app configuration or I can fork Fengari, make it build and just pull from the original repo every now and then. In both cases, I end up maintaining something: the webpack conf in the first case and the build script in the second. The beauty of create-react-app is that you can update it if you don't eject; if you eject, it's just a bunch of scripts that you have to maintain.

I was wondering whether it was possible to use fengari with create-react-app out of the box, since it's pretty standard to export a ES5-compiled dist of a library (all the major libraries do that, React included).

It seems it's not possible, so I'll fall back to writing additional code to support Fengari in my project.

@cristiano-belloni
Copy link
Author

cristiano-belloni commented Oct 2, 2017

I myself use webpack out of the box without issues.

It works, indeed. It's when it comes to minifying and source-mapping for production that UglifyJS dies.
create-react-app doesn't provide support for ES6 libraries because it expects a compiled version (it doesn't have to be a bundle. It can be simply a dist directory with separate files, 1 to 1 with src, transpiled with Babel. I did it with a Webpack bundle in the PR just because Webpack was already a dependency).

Right or wrong, it's pretty much what all the major Javascript libraries do. They have their src in Github and export a compiled ES5 dist directory in their npm package (excluding it in .gitignore). That's what you normally expect and what create-react-app expects. Obviously you're free to distribute your library the way you like, @daurnimator, but imho distributing ES6 code it's pretty unconventional and causes a bit of problems to developers who don't want to eject their configuration.

@giann
Copy link
Member

giann commented Oct 3, 2017

Closing this.
I think @daurnimator listed a pretty compelling list of arguments against any pre-compiled es5 version of fengari.

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