node-pool icon indicating copy to clipboard operation
node-pool copied to clipboard

Transpile to ES5 for use with webpack/uglify

Open tauren opened this issue 7 years ago • 5 comments

I'm using generic-pool to manage a pool of WebRTC data channels in a web project. This project uses webpack to bundle assets, which uses uglify to minify. The problem is that uglify dies on any non-ES5 code.

For instance, it dies when it hits the ES6 class keyword in DefaultEvictor:

ERROR in main.0211a87e1d77713c748a.js from UglifyJs
SyntaxError: Unexpected token: name (DefaultEvictor) [main.0211a87e1d77713c748a.js:24590,6]

It would be nice if babel was used to transpile to ES5 before publishing to npm. Both ES5 and ES6 can be published together, as the package.json properties main, browser, and jsnext:main can be used to target different environments. This way there wouldn't be any backward compatibility issues.

I need this immediately and am willing to work on a PR. Please let me know if you're willing to accept this as a contribution.

tauren avatar Feb 13 '17 21:02 tauren

Hi Tauren,

Thank you for effort you put into this. I am quite strongly against transpilation within this module, as our baseline target is node v4 and I favour simplicity, however I'd be down for doing something like https://github.com/jeffbski/joi-browser as a new repo/module which acts as a wrapper of sorts around joi, maybe called generic-pool-browser (see what I did there :-p ).

How does this sound?

sandfox avatar Feb 16 '17 12:02 sandfox

@sandfox

This is your project, so if you'd prefer that you or someone else create and maintain a separate generic-pool-browser project, that is your prerogative. I can certainly use a wrapping module instead. In fact, I had thought about doing just that so I didn't have to wait on generic-pool to be updated or to get my PR approved and merged. But I didn't go that way since I assumed you'd prefer to natively support multiple environments and that my wrapping module would ultimately have no purpose.

However, I have to say that I'm not convinced that is the better approach. Is there anything in this module that requires a node environment besides code syntax? It doesn't depend on fs or other node specific modules, does it?

I totally understand the desire to keep projects simple. But I'm also an advocate for supporting multiple environments if a project lends itself to doing so, as I believe generic-pool does. I'm currently successfully using my PR branch as an npm linked module with my web project. I certainly might be missing something, and if I am please let me know.

Also note that the changes I've proposed don't effect the source at all. It only adds a very simple babel build process that runs when you npm publish. This means you don't even need to have a dist folder committed within your repo.

tauren avatar Feb 16 '17 19:02 tauren

I now see that you are using EventEmitter from events. My app has a dependency on eventemitter3, but that wouldn't be automatically wired up. Perhaps there is more to getting generic-pool working in the browser. I'll have to look into why things are working in my webapp. I haven't tested my PR outside of my webpack/babel setup, which must be doing something to make it work.

tauren avatar Feb 16 '17 20:02 tauren

@sandfox Any thoughts on my comments? Should I be looking at creating a generic-pool-browser project myself? It wasn't clear to me from your response if you wanted to be part of maintaining it.

tauren avatar Feb 22 '17 10:02 tauren

Hi @tauren, sorry, been busy with very tedious end of company/tax paperwork :-( I'm totally down for creating / maintaininggeneric-pool-browser, infact it's here -> https://github.com/sandfox/node-pool-browser (the repo name is like that for consistency with the current one, but the module in npm should defo be called generic-pool-browser I think I might move the generic-pool stuff into it's own org as it's now at least three repos and could become more (but need to speak to other james about that first).

sandfox avatar Feb 23 '17 08:02 sandfox