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

Updated radium to 0.17, Update chai-enzyme #270

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

asvetliakov
Copy link
Contributor

Typings URL: [https://github.com/asvetliakov/typings-radium]
Source URL: [https://github.com/FormidableLabs/radium]

Typings URL: [https://github.com/asvetliakov/typings-chai-enzyme]
Source URL: [https://github.com/producthunt/chai-enzyme]

@unional
Copy link
Member

unional commented Apr 1, 2016

Thanks.
I'm not 100% sure, but I think the TestMode should be a var and not interface.
You can actually access it, right?

Also, maybe you can consider adding version in the typings.json for the version you are typings against.
I'm improving that part and if @blakeembrey agrees user can see what version it is for when they install your typings.

@asvetliakov
Copy link
Contributor Author

Yeah, var make sense, didn't try but actually import {TestMode} from "radium" should be allowed
@unional Should it be latest version? I.e "version": "0.17.0" ?

@unional
Copy link
Member

unional commented Apr 1, 2016

Yeah, var make sense, didn't try but actually import {TestMode} from "radium" should be allowed

Yes, you can import the interface
But you may not be able to do this:

import Radium from 'radium';

Radium.TestMode.disable();

@asvetliakov
Copy link
Contributor Author

@unional
Should be good now, please review

@unional
Copy link
Member

unional commented Apr 1, 2016

LGTM.
@blakeembrey, I think it can be merged.

@asvetliakov asvetliakov changed the title Updated radium to 0.17 Updated radium to 0.17, Update chai-enzyme Apr 1, 2016
@asvetliakov
Copy link
Contributor Author

@unional Sorry to bring another stuff into PR, but i updated chai-enzyme too

@unional
Copy link
Member

unional commented Apr 1, 2016

No problem. Sorry that will have to wait for Blake to merge PR. I'm not ready to merge other people's PR yet. :)

As for the typings. you can consider using declare namespace abc instead of declare module abc. That's the prefer way.

One more thing is I've updated the generator and it includes more features. You may like it (I hope) :)

@blakeembrey
Copy link
Member

@asvetliakov Can you augment the non-namespaced versions? It should work (let me know if it does not), but the tilde will break once you use it as a dependency: https://github.com/asvetliakov/typings-chai-enzyme/blob/master/index.d.ts#L2.

@asvetliakov
Copy link
Contributor Author

@blakeembrey I can't augment non tilde version, because current chai typings contains this:

declare module 'chai/lib/Assertion' {
export * from '~chai/lib/Assertion';
}

Module doesn't introduce local names and couldn't be augmented. Same for 'chai'.
How to augment non-namespaced version?

@blakeembrey
Copy link
Member

@asvetliakov When you bundle or it's installed, it should be re-writing the augmentation for you. I'll have to dive into it more at another time though. Definitely don't want to be augmenting the ~ version though, since that's entirely an implementation detail of Typings.

@asvetliakov
Copy link
Contributor Author

@blakeembrey Will not work, because if i do this:

import {Assertion} from "chai/lib/Assertion"
declare module "chai/lib/Assertion" {
// augmentation
}

it will produce this:

// Generated by typings
// Source: index.d.ts
declare module '~chai-enzyme/index' {
import {EnzymeSelector, ShallowWrapper, ReactWrapper, CheerioWrapper} from '~chai-enzyme~enzyme';
import {Assertion} from '~chai-enzyme~chai/lib/Assertion';

// Augment chai assertions
module '~chai-enzyme~chai/lib/Assertion' {

and when trying to use it:

import * as React from "react";
import * as chai from "chai";
import {expect} from "chai";
import * as chaiEnzyme from "chai-enzyme";
import {shallow} from "enzyme";

chai.use(chaiEnzyme());

class MyComponent extends React.Component<any, any> {
    render() {
        return (
            <div id="root">
                <h1>Test</h1>
                <span id="child">Child</span>
            </div>
        );
    }
}

let wrapper = shallow(<MyComponent />);

expect(wrapper).to.contain(<h1>Test</h1>); // error
expect(wrapper.find('span')).to.have.id('child'); // error
expect(wrapper).to.have.descendants('#child'); // error
expect(wrapper).to.have.id("root"); //error

because augmented module name is different than original chai's module

What is the reason to putting dependencies including into resulting type def? Versioning?

@asvetliakov
Copy link
Contributor Author

Did try to augment just 'chai', same result - the augmented name is being rewritten and thus isn't being augmented correctly.
Also, it bundles whole chai library into resulting file, and this producing additional error:

global {
  interface Object {
    should: A.Assertion;
  }
}
bundle.d.ts/main.d.ts (839,5): Duplicate identifier 'should'. (2300)
typings/main/definitions/chai/index.d.ts (500,5): Duplicate identifier 'should'. (2300)

@blakeembrey
Copy link
Member

Gotcha, I see now. If you switch to using peerDependencies, does that help you? In your case you don't want to augment a dependency because you want that dependency to be provided by your consumer - so peerDependency is the right concept here. Module names that can't be found aren't rewritten.

@unional
Copy link
Member

unional commented Apr 1, 2016

Btw if the chai typings has anything missing, let me know

@asvetliakov
Copy link
Contributor Author

Oh, ok, peerDependencies trick may work, let me try
On other node, can you point link on the main page to the current typings.json schema or description? I wasn't able to find it, though i remember it should be somewhere

@asvetliakov
Copy link
Contributor Author

@unional You need to export internal interfaces, i want to augment just 'chai' or 'chai/lib/Assertion' instead ~chai/lib/Assertion

@blakeembrey
Copy link
Member

@asvetliakov
Copy link
Contributor Author

Ok, with peerDependencies the chai isn't being included, but

import {Assertion, Match} from 'chai/lib/Assertion'

// Augment chai assertions
module 'chai/lib/Assertion' {
    interface Match {
        (selector: EnzymeSelector): Assertion;
    }
    interface Assertion {
...

This produces error because chai/lib/Assertion doesn't introduce local names: export * from '~chai/lib/Assertion'
Trying to augment just chai:

test/test.tsx (25,38): Property 'id' does not exist on type 'Assertion'. (2339)
test/test.tsx (26,25): Property 'descendants' does not exist on type 'Assertion'. (2339)
test/test.tsx (27,25): Property 'id' does not exist on type 'Assertion'. (2339)

I think this is because interface Assertion(and other interfaces, such as Match) isn't being exported from 'chai', instead it exports: export var Assertion: A.AssertionStatic;
ping @unional

@unional
Copy link
Member

unional commented Apr 1, 2016

Yeah, I need to expose the interfaces, for sure. We are debating what is the recommended approach on that. See typings/typings#354. What do you think?

@blakeembrey
Copy link
Member

@asvetliakov Interesting. I can make a patch to correctly rewrite the module '...', I suspect I have a dud module check there.

@blakeembrey
Copy link
Member

Maybe of interest @RyanCavanaugh, Typings has been using the proposed ~ hack since the 0.7 release but it's made things tricky when it comes to module augmentation. I don't want to merge public implementations of the hack for obvious reasons, should it be possible to augment the exported names?

@asvetliakov
Copy link
Contributor Author

@blakeembrey
You can try to use something like this
instead of

export * from "blablabla"

use

import bla = require("blablabla"); // or importa * as bla from "blablabla"
export = bla;

This way the module augmentation will work, i doubt that TS team will change something since this likely ES6 spec behavior (re-export doesn't introduce local names).

@blakeembrey
Copy link
Member

@asvetliakov I'll take a look, have you confirmed that fixes your issue though?

@asvetliakov
Copy link
Contributor Author

Yes, this works:

declare module 'chai/lib/Assertion' {
import * as bla from "~chai/lib/Assertion";
export = bla;
}

// in index.d.ts

declare module 'chai/lib/Assertion' {
  interface Assertion { ... }
}

@blakeembrey
Copy link
Member

Sweet, thanks. Just confirmed myself. Looks like I'll update to just use export and import from now on. My basic test case:

declare module '~x' {
  export interface Foo {
    x: string;
  }
}

declare module 'x' {
  // export * from '~x';

  import main = require('~x')
  export = main
}

declare module 'augment' {
  module 'x' {
    interface Foo {
      y: number
    }
  }
}

@asvetliakov
Copy link
Contributor Author

De nada. Looking forward for update 👍

@blakeembrey blakeembrey mentioned this pull request Apr 14, 2016
@blakeembrey
Copy link
Member

@asvetliakov Ready to update when you are. 1.0 has been released with the changes discussed - import = export = is the default alias now.

@asvetliakov
Copy link
Contributor Author

Thanks! Looks like this even won't be needed for 2.0
microsoft/TypeScript#8485

@blakeembrey
Copy link
Member

@asvetliakov Unless I'm missing something, that doesn't fix the augmenting of a module right?

@asvetliakov
Copy link
Contributor Author

If i'm correct with dev version we'll be able to augment just "chai" top level module even if it uses es6 reexport. need to test

@blakeembrey
Copy link
Member

blakeembrey commented May 18, 2016

Oh, that's not what I thought that PR was. I believe(d) it was to allow new names in global. E.g. declare global { const x: string }. Currently adding a new name in "global" does not work because of that restriction.

Edit: That's what I thought, but looking at the tests seems like you could be right. Seems interesting.

@asvetliakov
Copy link
Contributor Author

The PR for global augmentation is microsoft/TypeScript#8104

@blakeembrey
Copy link
Member

In this case, I can't tell if I should revert the import = export = syntax... Hopefully the TypeScript team can advise.

@blakeembrey
Copy link
Member

Thanks. Yeah, I haven't been following TypeScript for a bit 😄 Got a bit too much to follow.

@RyanCavanaugh
Copy link

We're removing the restriction that module augmentations can't introduce new top-level names, which I think is what the ~ workaround is about?

I'm working on a comprehensive set of docs that should address this and (hopefully) all other scenarios.

@blakeembrey
Copy link
Member

@RyanCavanaugh The ~ in Typings is to work around microsoft/TypeScript#6427. Otherwise people can't publish modules using Typings because TypeScript's node module resolution kicks in and it becomes invalid.

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

Successfully merging this pull request may close these issues.

4 participants