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

Auto generate TypeScript definition to allow chaining #884

Merged
merged 14 commits into from Jul 13, 2016

Conversation

Projects
None yet
5 participants
@ivogabe
Contributor

ivogabe commented May 27, 2016

I've implemented a generator that will create the TS definitions, with full support for chaining. Writing these type definitions by hand would be a lot of work or inaccurate. See the first comment in types/generator.js for details on how these definitions are created.

Fixes #871. Tests for this would be useful too, see #870.

@jamestalmage @SamVerschueren What do you think?

@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn

novemberborn May 29, 2016

Member

Haven't really looked at the generate script, but I think it'd be fine for it to use ES2015 (and thus require Node 6). It's something we'd have to run locally and then check in the results.

Member

novemberborn commented May 29, 2016

Haven't really looked at the generate script, but I think it'd be fine for it to use ES2015 (and thus require Node 6). It's something we'd have to run locally and then check in the results.

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe May 29, 2016

Contributor

I've addressed most of the comments and fixed the linting issues. Can you take another look?

Contributor

ivogabe commented May 29, 2016

I've addressed most of the comments and fixed the linting issues. Can you take another look?

Show outdated Hide outdated lib/runner.js
Show outdated Hide outdated types/generate.js
Show outdated Hide outdated types/generate.js
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus May 30, 2016

Member

Can you call the file generate.js => make.js instead and add a npm run script for it, like "make-ts": "node types/make.js".

And Travis is failing.

Member

sindresorhus commented May 30, 2016

Can you call the file generate.js => make.js instead and add a npm run script for it, like "make-ts": "node types/make.js".

And Travis is failing.

@SamVerschueren

This comment has been minimized.

Show comment
Hide comment
@SamVerschueren

SamVerschueren May 30, 2016

Collaborator

I would make the file executable. './make.js'

Collaborator

SamVerschueren commented May 30, 2016

I would make the file executable. './make.js'

@@ -198,3 +198,5 @@ Runner.prototype.run = function (options) {
return Promise.resolve(this.tests.build(this._bail).run()).then(this._buildStats);
};
Runner._chainableMethods = chainableMethods.chainableMethods;

This comment has been minimized.

@jamestalmage

jamestalmage Jun 1, 2016

Contributor

Should we wrap these options in a generator function? So we aren't exporting a mutable copy?

@jamestalmage

jamestalmage Jun 1, 2016

Contributor

Should we wrap these options in a generator function? So we aren't exporting a mutable copy?

This comment has been minimized.

@sindresorhus

sindresorhus Jun 3, 2016

Member

@jamestalmage Doesn't matter. It's for internal use only.

@sindresorhus

sindresorhus Jun 3, 2016

Member

@jamestalmage Doesn't matter. It's for internal use only.

@@ -0,0 +1,124 @@
'use strict';

This comment has been minimized.

@jamestalmage

jamestalmage Jun 1, 2016

Contributor

Shebang line and make it executable?

Does that even work on Windows?

@jamestalmage

jamestalmage Jun 1, 2016

Contributor

Shebang line and make it executable?

Does that even work on Windows?

This comment has been minimized.

@SamVerschueren

SamVerschueren Jun 2, 2016

Collaborator

👍 on this. I believe it will work as package-cli stuff also works...

@SamVerschueren

SamVerschueren Jun 2, 2016

Collaborator

👍 on this. I believe it will work as package-cli stuff also works...

This comment has been minimized.

@sindresorhus

sindresorhus Jun 3, 2016

Member

Haven't tried, but I don't think it will work. The reason you can use binaries in dependencies is that when npm install binaries it creates a "shim" on Windows that makes it behave like Unix.

@sindresorhus

sindresorhus Jun 3, 2016

Member

Haven't tried, but I don't think it will work. The reason you can use binaries in dependencies is that when npm install binaries it creates a "shim" on Windows that makes it behave like Unix.

This comment has been minimized.

@jamestalmage

jamestalmage Jun 3, 2016

Contributor

Yeah that was my concern. Let's just leave it as is.

@jamestalmage

jamestalmage Jun 3, 2016

Contributor

Yeah that was my concern. Let's just leave it as is.

@jamestalmage

This comment has been minimized.

Show comment
Hide comment
@jamestalmage

jamestalmage Jun 3, 2016

Contributor

Are we good here? This all looks good to me. @SamVerschueren - should I merge?

Contributor

jamestalmage commented Jun 3, 2016

Are we good here? This all looks good to me. @SamVerschueren - should I merge?

@SamVerschueren

This comment has been minimized.

Show comment
Hide comment
@SamVerschueren

SamVerschueren Jun 3, 2016

Collaborator

I did not test the output though but generally everything looks good. If you want me to test it more thoroughly, I'm ok with that. If you want to merge, that's ok as well. We can always finetune in follow up commits.

Great work @ivogabe!

Collaborator

SamVerschueren commented Jun 3, 2016

I did not test the output though but generally everything looks good. If you want me to test it more thoroughly, I'm ok with that. If you want to merge, that's ok as well. We can always finetune in follow up commits.

Great work @ivogabe!

@jamestalmage

This comment has been minimized.

Show comment
Hide comment
@jamestalmage

jamestalmage Jun 3, 2016

Contributor

Should we be storing generated.d.ts in the repo?

Why not add it to .gitignore and create it in a prepublish script?

Contributor

jamestalmage commented Jun 3, 2016

Should we be storing generated.d.ts in the repo?

Why not add it to .gitignore and create it in a prepublish script?

@SamVerschueren

This comment has been minimized.

Show comment
Hide comment
@SamVerschueren

SamVerschueren Jun 4, 2016

Collaborator

Good point. Don't see any downside of doing that.

Collaborator

SamVerschueren commented Jun 4, 2016

Good point. Don't see any downside of doing that.

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jun 4, 2016

Contributor

I usually don't add generated files to a repo, though I had two reasons now: it's still possible to install AVA from GitHub, and you can easily see what changed when you update the generator script.

Contributor

ivogabe commented Jun 4, 2016

I usually don't add generated files to a repo, though I had two reasons now: it's still possible to install AVA from GitHub, and you can easily see what changed when you update the generator script.

@jamestalmage

This comment has been minimized.

Show comment
Hide comment
@jamestalmage

jamestalmage Jun 4, 2016

Contributor

I'd prefer ignoring it if possible.

It would be awesome if npm had a "installed from GitHub" hook

Contributor

jamestalmage commented Jun 4, 2016

I'd prefer ignoring it if possible.

It would be awesome if npm had a "installed from GitHub" hook

@SamVerschueren

This comment has been minimized.

Show comment
Hide comment
@SamVerschueren

SamVerschueren Jun 5, 2016

Collaborator

Can it be executed on postinstall maybe? This would overwrite the generated.d.ts shipped with npm though.

Collaborator

SamVerschueren commented Jun 5, 2016

Can it be executed on postinstall maybe? This would overwrite the generated.d.ts shipped with npm though.

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jun 8, 2016

Contributor

I've added it to .gitignore. postinstall won't work, since the generator script requires NodeJS 6. I think that TS users already know that it's usually not possible to install a package from GitHub. Do you agree?

Contributor

ivogabe commented Jun 8, 2016

I've added it to .gitignore. postinstall won't work, since the generator script requires NodeJS 6. I think that TS users already know that it's usually not possible to install a package from GitHub. Do you agree?

@jamestalmage

This comment has been minimized.

Show comment
Hide comment
@jamestalmage

jamestalmage Jun 8, 2016

Contributor

I think that TS users already know that it's usually not possible to install a package from GitHub.

I say we start with it this way, and see if there are lots of complaints.

Contributor

jamestalmage commented Jun 8, 2016

I think that TS users already know that it's usually not possible to install a package from GitHub.

I say we start with it this way, and see if there are lots of complaints.

Show outdated Hide outdated package.json
@jamestalmage

This comment has been minimized.

Show comment
Hide comment
@jamestalmage

jamestalmage Jun 13, 2016

Contributor

I think that script generation should happen in a prepublish script. Otherwise, I think we are good to go.

Contributor

jamestalmage commented Jun 13, 2016

I think that script generation should happen in a prepublish script. Otherwise, I think we are good to go.

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jun 15, 2016

Contributor

@jamestalmage I would agree, but I'm afraid that the package cannot be published anymore with that change... The generation script uses ES2015, thus needs Node 6, and npm publish is broken on Node 6.

Contributor

ivogabe commented Jun 15, 2016

@jamestalmage I would agree, but I'm afraid that the package cannot be published anymore with that change... The generation script uses ES2015, thus needs Node 6, and npm publish is broken on Node 6.

@SamVerschueren

This comment has been minimized.

Show comment
Hide comment
@SamVerschueren

SamVerschueren Jun 15, 2016

Collaborator

@ivogabe I can't find features that can only be ran on Node 6 (spread operators for instance). Node 4 supports the majority of those features as well.

Collaborator

SamVerschueren commented Jun 15, 2016

@ivogabe I can't find features that can only be ran on Node 6 (spread operators for instance). Node 4 supports the majority of those features as well.

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jun 15, 2016

Contributor

That makes life easier then. I've added it in 54a8324.

Contributor

ivogabe commented Jun 15, 2016

That makes life easier then. I've added it in 54a8324.

@jamestalmage

This comment has been minimized.

Show comment
Hide comment
@jamestalmage

jamestalmage Jun 15, 2016

Contributor

You need to polyfill .includes. I see no reason not to just require('babel-polyfill') for this.

Contributor

jamestalmage commented Jun 15, 2016

You need to polyfill .includes. I see no reason not to just require('babel-polyfill') for this.

ivogabe added some commits Jun 16, 2016

// `after.cb` is fully written, and `cb.after` is emitted as an alias
// using `typeof after.cb`.
const path = require('path');

This comment has been minimized.

@jamestalmage

jamestalmage Jun 18, 2016

Contributor

const => var throughout. It's not supported on Node 0.12 or 0.10.

Hopefully, that's the last of it. I would like to get this merged.

@jamestalmage

jamestalmage Jun 18, 2016

Contributor

const => var throughout. It's not supported on Node 0.12 or 0.10.

Hopefully, that's the last of it. I would like to get this merged.

This comment has been minimized.

@sindresorhus

sindresorhus Jun 18, 2016

Member

Same with arrow functions, let, etc, used here. Would be much easier to just require('babel/register') this script.

@sindresorhus

sindresorhus Jun 18, 2016

Member

Same with arrow functions, let, etc, used here. Would be much easier to just require('babel/register') this script.

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus
Member

sindresorhus commented Jun 29, 2016

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jul 1, 2016

Contributor

Sorry, I've been too busy lately and forgot about this. I've added babel/register, I thought it was easiest to do that in package.json, otherwise I would need to add another file that required babel and the generation script.

Contributor

ivogabe commented Jul 1, 2016

Sorry, I've been too busy lately and forgot about this. I've added babel/register, I thought it was easiest to do that in package.json, otherwise I would need to add another file that required babel and the generation script.

Show outdated Hide outdated package.json
@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jul 13, 2016

Contributor

Does babel-node require more configuration? It still fails on node 0.10 and 0.12. The tests are failing on node 4, has anyone an idea why?

Contributor

ivogabe commented Jul 13, 2016

Does babel-node require more configuration? It still fails on node 0.10 and 0.12. The tests are failing on node 4, has anyone an idea why?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Jul 13, 2016

Member

@ivogabe Unfortunately yes, since Babel 6 everything needs config.

Replace it with:

"make-ts": "babel-node --presets=babel-preset-es2015 --plugins=transform-runtime types/make.js"
Member

sindresorhus commented Jul 13, 2016

@ivogabe Unfortunately yes, since Babel 6 everything needs config.

Replace it with:

"make-ts": "babel-node --presets=babel-preset-es2015 --plugins=transform-runtime types/make.js"

@sindresorhus sindresorhus merged commit 1dd6d5b into avajs:master Jul 13, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 96.692%
Details
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Jul 13, 2016

Member

Awesome! Thanks for working on this @ivogabe :)

celebration

Member

sindresorhus commented Jul 13, 2016

Awesome! Thanks for working on this @ivogabe :)

celebration

@SamVerschueren

This comment has been minimized.

Show comment
Hide comment
@SamVerschueren

SamVerschueren Jul 13, 2016

Collaborator

Nice work @ivogabe !

Collaborator

SamVerschueren commented Jul 13, 2016

Nice work @ivogabe !

@ivogabe

This comment has been minimized.

Show comment
Hide comment
@ivogabe

ivogabe Jul 13, 2016

Contributor

🎉

Contributor

ivogabe commented Jul 13, 2016

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment