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

Compiled version uses ES6 syntax (=>) #21

Open
ffdead opened this issue Feb 9, 2017 · 21 comments
Open

Compiled version uses ES6 syntax (=>) #21

ffdead opened this issue Feb 9, 2017 · 21 comments

Comments

@ffdead
Copy link

ffdead commented Feb 9, 2017

Thanks for a nice lib!

Seems like the babel transpilation step is not working correctly, should there really be "=>" in the output file?

./mixwith.js:

const Cached = exports.Cached = mixin => wrap(mixin, superclass => {
@minecrawler
Copy link

There also is const and mixwith works with class (see examples), so I do not see how arrow-functions should be a problem?

@ffdead
Copy link
Author

ffdead commented Feb 9, 2017

Hi @minecrawler

I would like to avoid enabling babelify transpilation globally on all imported node_modules. Most modules provide an ES5 version as default import so it would be nice if this did too (or at least some way to import an ES5 version even if it's not default).

Currently my build breaks in the uglify step after adding this module:

uglifyjs node_modules/mixwith/mixwith.js                                                                                                               
Parse error at node_modules/mixwith/mixwith.js:26,48
Unexpected token: operator (>)
Error
    at new JS_Parse_Error (/usr/local/lib/node_modules/uglify-js/lib/parse.js:196:18)
    at js_error (/usr/local/lib/node_modules/uglify-js/lib/parse.js:204:11)
    at croak (/usr/local/lib/node_modules/uglify-js/lib/parse.js:663:9)
    at token_error (/usr/local/lib/node_modules/uglify-js/lib/parse.js:671:9)
    at unexpected (/usr/local/lib/node_modules/uglify-js/lib/parse.js:677:9)
    at expr_atom (/usr/local/lib/node_modules/uglify-js/lib/parse.js:1173:9)
    at maybe_unary (/usr/local/lib/node_modules/uglify-js/lib/parse.js:1334:19)
    at expr_ops (/usr/local/lib/node_modules/uglify-js/lib/parse.js:1369:24)
    at maybe_conditional (/usr/local/lib/node_modules/uglify-js/lib/parse.js:1374:20)
    at maybe_assign (/usr/local/lib/node_modules/uglify-js/lib/parse.js:1398:20)

@rantecki
Copy link

+1 Having same issue with uglify.

@ajoslin103
Copy link

+1

mine broke too, even after babelify

broke here: const wrap = exports.wrap = (mixin, wrapper) =>

@Download
Copy link

Download commented Apr 6, 2017

Yup. The current 'standard' is:

  • Publish an ES5 or ES2015/ES6 'light' version which should run natively under (current) Node JS (this means no import/export) under the "main" key in package.json
  • Also publish a version that is basically ES2015 (ES6) (so including import/export) for consumption by bundlers like Webpack 2 and Rollup under the "module" key in package.json

How 'low' to go in transpiling down for the "main" entry depends on requirements/desire to support older Node / browser versions, but most people still go with ES5 here because it's just most convenient. People that use bundlers will get ES6 code that does not need to be transpiled down, but does still need to be bundled (because most browsers and node don't support import/export natively).

See this proposal:
In Defense of .js
which is implemented by Webpack and Rollup.

@pstricker
Copy link

+1 webpack + babel

@cellvia
Copy link

cellvia commented Apr 6, 2017

what a PITA ... had to remove the modules babelrc,and manually copy it into my app :-/ otherwise great module...

@ajoslin103
Copy link

I had to stop using this module because the module version used ES6

Perhaps the author would accept PR #20 ?

@Download
Copy link

@ajoslin103 I built a library, mics, inspired by mixwith, because I had issues working with mixwith same as you. Maybe it could be something for you? It's an evolution of the ideas in mixwith (and in Justin's excellent blog post on real mixins).

@ajoslin103
Copy link

ajoslin103 commented Apr 17, 2017 via email

@Download
Copy link

Download commented Apr 17, 2017

thanks - will try it out

@ajoslin103 That would be great! Love to hear what you think!

I have to warn you it's not completely stable (as far as API is concerned, code works). I am working on 0.5 atm and there will be some changes (for the good!). But I would be glad if you would try it and give me some feedback in the issues as I work towards 1.0.

I expect to get 0.5 up soon(ish) :)

@gryphonmyers
Copy link

I forked the repo and added a babelify transform since @justinfagnani is unresponsive. This works for my purposes. I published it to NPM as mixwith-es5. PRs welcome to get it working with other build setups. https://github.com/gryphonmyers/mixwith.js

@justinfagnani
Copy link
Owner

That's fine I suppose, but the ES5 version won't work with ES6 superclasses, so I don't really see the point. ES5 can't emulate super(), so as soon as you apply a mixin to an ES6 class, you'll get an error.

At this point libraries, especially ones that use ES6 features, should just distribute ES6 and let applications compile down to something else if needed. All browsers and Node support ES6 now.

@gryphonmyers
Copy link

To clarify, all I added was a transform flag to the package.json, which tells bundlers (browserify, in my case) "if you're using babel, use babel on this module."

@Download
Copy link

@justinfagnani Great to hear from you! Your post was a great inspiration, thanks for writing it!

@rmckeel
Copy link

rmckeel commented Jan 22, 2018

Would love to see this issue fixed in this repo. For now, I'll switch to another forked repo.

FWIW, I don't agree with the comment "All browsers and Node support ES6 now." For me, Mixwith caused a compilation issue (an old < v3 UglifyJsPlugin with webpack broke on it), then is causing an IE 11 issue. Based on https://netmarketshare.com/browser-market-share.aspx, IE 11 had a 9.45% market share in December 2017. In my experience, many enterprise organizations still rely on it, and those who build applications for enterprise must consider that market segment.

That being said, other than not clearing out Git issues, I appreciate Mixwith. It is an excellent approach to writing DRYer TypeScript code, thanks for the implementation Justin!

@justinfagnani
Copy link
Owner

Like I've tried to point out before, a compiled version of this library simply does not work with classes. I really don't think it's work distributing a broken version of the library.

I'm not in any way suggesting that IE11 support isn't important, but figuring out what language features need to be compiled out is really the job of the application that knows what language features are supported by target platforms. Running dependencies through babel-preset-env is a great way for an app to only compile out what's necessary, and it ensures that if you're compiling out classes in mixwith, you're also compiling out classes anywhere it might be applied, so that the whole app continues to work.

@rmckeel
Copy link

rmckeel commented Jan 22, 2018

@justinfagnani Thanks for weighing in, I believe I understand the conundrum now.

Since IE11 is a project requirement, I will back off from this library and (re-)adjust my patterns to an ES5-workable approach. But, great coding patterns, thanks for sharing your code and ideas in this area.

@gryphonmyers
Copy link

@rmckeel it feels wrong but I ended up working around the es6 issues by doing global transforms and excluding extensions or paths where necessary. I think @justinfagnani is actually kinda right - the burden of transpiling JS should be on the implementing app. It might be convenient if a pre-transpiled version was included in this repo, but by retaining complete control of the transpilation step, you can be more consistent about which features you do and don't transpile.

@rmckeel
Copy link

rmckeel commented Jan 23, 2018

@gryphonmyers @justinfagnani Thanks for the thoughts.

I ended up importing mixwith, then transpiling it to ES5 with the rest of the app code, and it is now working on IE11 as well, with super() calls (simple, no arguments). I may be missing something, since I thought that couldn't be done - perhaps I'll find limitations when I use it in a more sophisticated manner.

Key pieces of code attached for posterity, with APIService being an abstract class that is extended by MockAPI and API, then injected into the Angular 4 app.

        {
            // using an abstract class-interface APIService, and injecting with proper API service,
            // @help https://angular.io/guide/dependency-injection-in-action#class-interface
            provide: APIService,
            useClass: (GeneralConfig.mockAPIData) ? MockAPI : API
        }

export const APIMixin = Mixin((superclass) => class extends (superclass as new() => {}) { ... }
export class API extends (mix(APIService).with(APIMixin) as new() => {}) { ... }

I'm sure this can be improved, but I'm happy as it is now working as originally built across all needed platforms.

@bZichett
Copy link

bZichett commented Oct 19, 2019

I was seeing an issue when minifying a webpack bundle with this library, due to it having es6 syntax. The easiest way to fix it for me was just to copy the source file into my repo and I was able to compile with no special configuration that targets es6 => es5 compilation for this specific file in the node_modules directory

Since the library hasnt updated in 4 years, I suppose this is fine.

Anyway, this is a great, simple module but figuring out it was the cause took a little of my time (~30 mins) since I didnt create a production build for quite some time after incorporating it.

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