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 type definitions for use in TypeScript #60

Merged
merged 1 commit into from
Oct 26, 2016

Conversation

tbekolay
Copy link
Collaborator

Hello! Thanks for brace, it's been very useful in our application, which we build with webpack.

We've recently ported over our codebase from raw JS to TypeScript, and found that the available type definitions don't work with brace out of the box. This commit adds definitions that work, generated by a similar process as brace uses to generate JS source (see the commit message for more details).

I chose to do this with a separate update-ts.js script exposed through an npm update-ts command, but if you prefer I could instead add the contents of update-ts.js to the existing update.js.

Alternatively, if you'd prefer that type definitions live outside of this repository, I'm happy to make a separate repository for this. However, I'm submitting this PR first as it's nice to be able to npm install brace and have the type definitions automatically available without the extra step of downloading them with typings.

@thlorenz
Copy link
Owner

thlorenz commented Aug 16, 2016

Nice! this looks good to me except for two things:

  • we should update the ts definitions when doing npm run update as I can't see a reason why you'd update the code but not the typescript definitions, to that end just change the update script to "update": "(cd build && node ./update && node ./update-ts)",
  • explain something in the Readme about the typescript definitions so people understand what they are, why they are there and how to use them

Thanks for your work! (not a typescript user myself, but I bet this will help lots of others that are)

@tbekolay
Copy link
Collaborator Author

Sounds good! I'll update this PR tomorrow to include those two things.

@thlorenz
Copy link
Owner

@tbekolay made you collaborator.

So once you make those changes an I reviewed them, please merge to master following this guide.
I will then version and publish to npm.

Thanks.

@tbekolay
Copy link
Collaborator Author

tbekolay commented Aug 17, 2016

I made the requested changes in a fixup commit (ada4a6f) so you can see the diff. Before fast-forward merging, I will squash these two commits into one commit (we also avoid merge commits in our project, so these steps are familiar to me).

@thlorenz
Copy link
Owner

LGTM, please merge to master.
I'll pub a new version.

And sorry for the long delay

@thlorenz
Copy link
Owner

BTW one thing I'm not a huge fan of is the code duplication in the update scripts.
I.e. when we fix a bug in one ppl who don't care about typescript (that'd be me as well) will fail to fix it there as well.
Ideally we'd pull some of this out into reusable functions which both updaters pull in and use.

@tbekolay
Copy link
Collaborator Author

I'm not a huge fan of is the code duplication in the update scripts.

Yes, I agree this is not ideal. I'll merge for now, but I'll add reducing duplication to my todo list.

Previously, brace did not work well in TypeScript because the
existing type definitions for the Ace editor from DefinitelyTyped
are written assuming a global "ace" variable, rather than
being modular and thus importable like so:

    import * as ace from "brace";

Additionally, the changes that brace makes (mostly the acequire
rename) are not reflected in those definitions.

This commit adds a script to obtain the current type definitions
from DefinitelyTyped and rewrite them in a similar way as the
Ace source itself is rewritten in brace. The resulting
type definitions are exposed to TypeScript through the "typings"
key in package.json (though since they live in `index.d.ts`, they
would usually be discovered automatically).
@tbekolay tbekolay merged commit a94b9d1 into thlorenz:master Oct 26, 2016
@tbekolay tbekolay deleted the typings branch October 26, 2016 18:31
@thlorenz
Copy link
Owner

Thanks published as 0.9

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.

2 participants