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

Add Babel #15

Closed
wants to merge 3 commits into from
Closed

Add Babel #15

wants to merge 3 commits into from

Conversation

hzoo
Copy link
Contributor

@hzoo hzoo commented Jun 19, 2016

Ref #9

Just add's Babel, but with preset-env

Old description

- add `make build` and `npm run build` - rename src files to `src` instead of `lib`

Just did the basic thing, probably needs changes, might need to look again monday/tues though since I'm busy for most of tomorrow.

These tests were failing for me locally for some reason (node 6) on master as well

  gyppies
    1) should build copies
    2) should build executable
    3) should build loadable_module
    4) should build shared_lib
    5) should build static_lib

     Error: spawnSync ninja ENOENT
      at exports._errnoException (util.js:1007:11)
      at spawnSync (child_process.js:464:20)
      at Context.it (test/gyppies-test.js:28:9)

bin/gyp Outdated
@@ -1,7 +1,7 @@
#!/usr/bin/env node
const gyp = require('../');
var gyp = require('../');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not compiling bin/ and since its just a few lines I just changed to var

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hzoo hzoo changed the title Add Babel WIP: Add Babel Jun 19, 2016
@indutny
Copy link
Owner

indutny commented Jun 19, 2016

Thank you! Tests require ninja binary in a PATH. It could be installed from https://ninja-build.org/ or built from source.

@hzoo
Copy link
Contributor Author

hzoo commented Jun 19, 2016

Cool just did brew install ninja and it's good. Actually found a bug with babel haha regarding const/scoping, which I reduced to

function Ninja() {}
Ninja.prototype.copies = function copies() { // SyntaxError: "copies" is read-only
  const copies = 1; // 
};

Ok odd it's getting Error: Cannot find module '../lib/gyp/generator/ninja/writer' on CI (works locally now)

Need to add transform-runtime for Map etc

@hzoo hzoo force-pushed the add-babel branch 4 times, most recently from a8152c7 to 236db66 Compare June 19, 2016 04:23
@hzoo hzoo changed the title WIP: Add Babel Add Babel Jun 19, 2016
@indutny
Copy link
Owner

indutny commented Jun 19, 2016

Thank you so much for this!

I'm a complete noob with regards to Babel. As far as I got it, it looks like require('gyp.js') will always work with a transpiled version. Is this correct? Wouldn't it be slower than what we currently have?

Unfortunately there are no benchmarks for the input at the moment, so I guess it could be benchmarked only on node.gyp.

@kzc
Copy link

kzc commented Jun 19, 2016

Wouldn't it be slower than what we currently have?

Idiomatic ES6 is typically slower than hand-written ES5 on v8 as it is not as well tuned.

https://kpdecker.github.io/six-speed/
http://www.incaseofstairs.com/2015/06/es6-feature-performance/
https://news.ycombinator.com/item?id=11203183

for/of loops, for example, should be avoided.

@indutny
Copy link
Owner

indutny commented Jun 19, 2016

@kzc so basically, the things that I have used here are: const/let, arrow functions, and some String# methods.

@indutny
Copy link
Owner

indutny commented Jun 19, 2016

@hzoo I have simplified lots of code in recent commits. I think we now use only let and const from ES6. Should be much simpler for a Babel now 😉

@hzoo
Copy link
Contributor Author

hzoo commented Jun 20, 2016

Adding more or less ES6 is fine for Babel 😄 (given i'm running the es2015 preset which transpiles es6 syntax, otherwise could choose a smaller/specific set of transforms). Still have to transpile the use of stuff like Map/Set though (either a specific module like es6-map or use the babel one (core-js))

@indutny
Copy link
Owner

indutny commented Jun 20, 2016

@hzoo yeah, I just hope to gain as much control as possible over the resulting code. Optimally, it should be almost the same, but with let/const replaced by vars and => replaced by functions. This way, we can have some kind of expectations of resulting performance.

@hzoo
Copy link
Contributor Author

hzoo commented Jun 20, 2016

You can use "loose" mode for most things but ok, what do you want to do for Map/Set? There's http://babeljs.io/docs/plugins/transform-runtime/ (uses https://github.com/zloirock/core-js) or another polyfill

@indutny
Copy link
Owner

indutny commented Jun 20, 2016

@hzoo I think loose mode is what we actually need indeed. I have no idea what polyfills are best, its up to you! 👍

@hzoo
Copy link
Contributor Author

hzoo commented Jul 2, 2016

Sorry didn't get to update this. When I rebased, I realize that parser-base doesn't work on node 0.10 either 😛 (Can either run babel on that too.. or just paste that code in the babel repl and replace)

Also do we want a polyfill that changes globals (Map, Set) or just use the ponyfill's modules?

edit: Not sure why the commits aren't updating. branch is here: https://github.com/hzoo/gyp.js/tree/add-babel (can do another pr)

@hzoo
Copy link
Contributor Author

hzoo commented Jul 2, 2016

yeah, I just hope to gain as much control as possible over the resulting code.

Also regarding your earlier comment, babel is pretty good on the output of most things (especially with loose mode). Like classes is no big deal (you can try it in the repl here). In that case, the only thing it adds that you wouldn't do handwritten is the classCallCheck to make sure you call with new. Basically destructuring is essentially the same too

so let/const, template strings, destructuring, classes, arrow functions, shorthand methods/properties, computed properties, literals are simple.

@hzoo
Copy link
Contributor Author

hzoo commented Jul 19, 2016

@indutny should I still update this? Not sure why the commits are off now (maybe force pushed incorrectly)

@indutny
Copy link
Owner

indutny commented Jul 20, 2016

@hzoo TBH, I'm not yet sure. I think that I have reduced ES6 usage to get this thing working on node.js v4-master.

At the moment, @pmed is pushing gyp.js as a backend for node-gyp, and I would like to keep this a priority for this project.

I'm really glad to see lots of things that you did here. Let's return to this a bit later, when we will reach the node-gyp goal.

Thanks!

@hzoo
Copy link
Contributor Author

hzoo commented Jul 20, 2016

Ok rebased again so it only has my changes. it fails on node 0.10 because some of the dependencies are on node 4

@hzoo
Copy link
Contributor Author

hzoo commented Mar 14, 2017

@indutny should I close this PR? It's been a long time but it's a lot easier to do this now with babel-preset-env, by targeting node 0.10/0.12/4/etc (it only compiles what's not supported)

{
  "presets": [
    ["env", {
      "targets": {
        "node": 4
      }
    }]
  ]
}

@indutny
Copy link
Owner

indutny commented Mar 17, 2017

Actually, I have been playing with babel lately and I think I'm finally ready to accept this 👍

Would you care to rebase this PR, please?

@hzoo
Copy link
Contributor Author

hzoo commented Mar 17, 2017

Ok I rebased and am targeting node 4 (since now 0.10/0.12 are deprecated) if that is what we wanted now

@indutny
Copy link
Owner

indutny commented Mar 17, 2017

Landed in 1ad257b, thank you so much! This should give me some starting base to porting this to duktape.

@indutny indutny closed this Mar 17, 2017
@hzoo
Copy link
Contributor Author

hzoo commented Mar 23, 2017

Do we close #9 now?

@indutny
Copy link
Owner

indutny commented Mar 23, 2017

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

Successfully merging this pull request may close these issues.

3 participants