Compiled version uses ES6 syntax (=>)

ffdead commented 7 years ago

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 commented 7 years ago

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

ffdead commented 7 years ago

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 commented 7 years ago

+1 Having same issue with uglify.

ajoslin103 commented 7 years ago

+1

mine broke too, even after babelify

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

Download commented 7 years ago

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 commented 7 years ago

+1 webpack + babel

cellvia commented 7 years ago

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

ajoslin103 commented 7 years ago

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

Perhaps the author would accept PR #20 ?

Download commented 7 years ago

@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 commented 7 years ago

thanks - will try it out

Al;

- There’s not enough room in this Handbasket !!

On Apr 17, 2017, at 7:12 PM, Stijn de Witt notifications@github.com wrote:

@ajoslin103 https://github.com/ajoslin103 I built a library, mics https://github.com/download/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).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/justinfagnani/mixwith.js/issues/21#issuecomment-294621780, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbF9deO0AECnemzuz1SXht1QH9wxsweks5rw_HOgaJpZM4L72Hg.

Download commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

rmckeel commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 5 years ago

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.

justinfagnani / mixwith.js

Compiled version uses ES6 syntax (=>) #21