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

Added exports for chai extensions #3

Closed
wants to merge 0 commits into from

Conversation

olivr70
Copy link

@olivr70 olivr70 commented Mar 19, 2016

Changes required for the chai-as-promised type definition

@@ -10,4 +10,6 @@ import {Chai} from './lib/Chai';

declare var chai: Chai;

export = chai;
export default chai;
Copy link
Member

Choose a reason for hiding this comment

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

This change can't be accepted, it's not correct.

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be merged it you don't use this and remove the Chai interface instead actually. cc @unional, does that sound right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. The other things is about the tslint. The format might be a bit off.

@blakeembrey
Copy link
Member

@olivr70 The changes can not be accepted because you're changing to export incorrectly.

@unional The changes make sense, it would be good to have some proper refactoring to not export an interface as the main and instead have multiple exports. Currently nothing can be augmented for re-used because of the type definition structure.

export declare var config:Config;
export declare var util:Utils;
export function should(): Should;
export function use(fn: ((chai: any, utils: Utils) => void) | ((chai: any) => void) ): Chai;
Copy link
Member

Choose a reason for hiding this comment

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

use (fn: ChaiExtension): Chai? I'd also move this to the main export to be able to kill the Chai name. It'll then look like import * as Chai from './lib/Chai'.

@olivr70
Copy link
Author

olivr70 commented Mar 19, 2016

Thank you for your comments. As you have guessed, I am rather new to this ecosystem.

The olivr70/typings-chai-as-promised project references my version of npm-chai, compiles and runs multiple mocha tests (in the test folder : just clone and run npm test).

I don't understand why "export default chai;" is not correct (it is compiled by the Typescript compiler)

My problem for chai-as-promised are :

  • that I have to augment inner modules whose naming is linked to typings implementation details (like ~chai/lib/Assert)
  • is has to be an ambient module to augment the ambient modules generated by typings

@unional
Copy link
Collaborator

unional commented Mar 19, 2016

Definitely should have multiple exports. When I port them I focus on cleaning up and expose the core shape.

@unional
Copy link
Collaborator

unional commented Mar 19, 2016

I don't understand why "export default chai;" is not correct

That is because the source package (chai) is a commonjs and export as modules.export =.
export default is ES6, which chai is not a ES6 package.

@unional
Copy link
Collaborator

unional commented Mar 19, 2016

that I have to augment inner modules whose naming is linked to typings implementation details (like ~chai/lib/Assert)

Unfortunately I think currently TypeScript 1.8 has a bug on the module augmentation, so it might not work.

@olivr70
Copy link
Author

olivr70 commented Mar 21, 2016

@blakeembrey about the export default chai; I get the argument that this is ES6 style, but because we are writing a .d.ts file, this will be used only by the Typescript compiler and never appear in the generated .js file.

And the Typescript spec (section 11.3.4.2 does not mention any limit to using default.

Am I missing something ?

@unional
Copy link
Collaborator

unional commented Mar 21, 2016

@olivr70 , it's a whole can of worms. Don't go there: microsoft/TypeScript#7398

@blakeembrey
Copy link
Member

I get the argument that this is ES6 style, but because we are writing a .d.ts file, this will be used only by the Typescript compiler and never appear in the generated .js file.

If you write a definition that does not match the source code, you can never expect runtime to actually work. By writing it incorrectly, people won't be able to actually use the definition. At least, they will, but not until they actually try to run their code.

@blakeembrey
Copy link
Member

For example, if you say the source code is export default (because that's what a .d.ts, the typed .js) then people need to use import chai from '' syntax. However, once it goes to runtime land it all explodes because Chai does not have an exported member called default.

@blakeembrey
Copy link
Member

Here's a simple example: http://www.typescriptlang.org/Playground#src=export%20default%20function%20()%20%7B%7D%3B. Notice exports.default = default_1 in the generated code. That's the equivalent here. If you try to do that to Chai, which does not have a exports.default, the runtime (JavaScript) will never run because .default is not a property on the Chai export. Does that help?

@unional
Copy link
Collaborator

unional commented Mar 25, 2016

@olivr70 are you working on another PR? I can re-work it to expose the interfaces.

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