emmetio / codemirror-plugin

Emmet plugin for CodeMirror web editor
MIT License
74 stars 12 forks source link

Minifying with uglify fails because build doesn't target ES5 #4

Closed tonyonodi closed 4 years ago

tonyonodi commented 6 years ago

I'm attempting to use this project in a project that uses Create React App, which uses Uglify when building. When I try to build I get the following error message

Failed to compile.

Failed to minify the code from this file: 

        ./node_modules/@emmetio/stream-reader/dist/stream-reader.es.js:4 

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

The link in the error message is very instructive and explains the issue well. Basically Uglify needs code that is compiled to ES5 to work and codemirror-plugin doesn't do this

I have already forked codemirror-plugin and its dependencies and have begun the process of modifying them to target ES5, but it's slow going because I'm not familiar with rollup or mocha and I'm unable to get any of the tests to pass and I don't know if what I have at the end of it will be good enough to merge into this project.

bradwestfall commented 6 years ago

Yep, getting the same thing. Came here after reading https://github.com/facebookincubator/create-react-app/issues/1418#issuecomment-274131392

bradwestfall commented 6 years ago

@sergeche Is there any way to solve this? Or maybe make it so It can be used on a CDN like https://unpkg.com/

sergeche commented 6 years ago

Not familiar with Create React App build process, but working solution is to preprocess libs with ES6 code with Babel/Buble or use uglify-es

bradwestfall commented 6 years ago

@sergeche Create React App's build process uses uglify under the hood and I'm not sure if it was a decision of CRA to configure it this way, but they don't transpile dependencies and they don't support ES6+ dependencies stating that:

ES6 syntax in dependencies is currently not supported because we use Uglify. This will be fixed when we switch to Babili when it's more stable.

Nevertheless it's important to stress that we aren't and won't be transpiling any dependencies. This is intentional. If your dependencies are published in ES6, your app will not work in browsers that don't support ES6.

Create React App can only be customized after "ejecting" to gain access to babel and webpack configs - which I did and tried to customize to transpile emmet, but with emmet's dependencies also being es6, it turned into a bit of a mess. Any ideas?

sergeche commented 6 years ago

You just need to transpile packed bundle before passing it to uglify. If you use Rollup, simply add Babel or Buble into plugin list. This is how it works for bundling standalone minified CodeMirror plugin: https://github.com/emmetio/codemirror-plugin/blob/master/rollup.config.js#L22

It also can be done in Webpack but I’m not familiar with it.

tonyonodi commented 6 years ago

@bradwestfall I've come up with a workaround for now. I've forked the project and changed the build process to use Node Resolve and Buble and committed ./dist so I can add it straight to my project without distributing on npm.

Using Node Resolve has increased the size of the dist file by quite a bit, but I don't think this matters because as far as I can tell the dependency tree consists entirely of other emmetio packages, so not anything that's likely to be used elsewhere in a project. Does that sound right to you, @sergeche?

You should be able to get your project building by running

npm uninstall --save @emmetio/codemirror-plugin
npm install --save https://github.com/tonyonodi/codemirror-plugin.git
bradwestfall commented 6 years ago

@tonyonodi I appreciate the work, but if I use your fork I won't get the same maintenance updates as I would from @emmitio. I'm trying to solve this with Webpack and Babek which I realize is the opposite tools that it seems like you guys are familiar with, but I just can't get it working. I've tried telling Babel in my setup to include the node_modules/@emmet... stuff like this and it doesn't work for me

{
  test: /\.(js|jsx)$/,
  include: [
    paths.appSrc,
    path.resolve(process.cwd(), 'node_modules/@emmetio/codemirror-plugin'),
    path.resolve(process.cwd(), 'node_modules/@emmetio/stream-reader-utils'),
    path.resolve(process.cwd(), 'node_modules/@emmetio/stream-reader'),
    path.resolve(process.cwd(), 'node_modules/@emmetio/field-parser')
  ],
  loader: require.resolve('babel-loader')
}

Btw, paths.appSrc comes from Creact React App and I started out by just adding path.resolve(process.cwd(), 'node_modules/@emmetio') and variations like path.resolve(process.cwd(), 'node_modules/@emmetio/**/*') but it would snag on some file still. So I started adding files one at a time like the list above and it seemed to work (with each file added to the list, it would then get past that file and stag on another file), but that only lated until field-parser then this strategy stopped working. Just adding this here for documentation. Not sure if we'll get this solved

sergeche commented 4 years ago

I guess it’s not relevant anymore, closing