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

fix module export for class #135

Merged
merged 3 commits into from
Feb 7, 2018
Merged

fix module export for class #135

merged 3 commits into from
Feb 7, 2018

Conversation

aoberoi
Copy link
Contributor

@aoberoi aoberoi commented Feb 3, 2018

As described in #104, I was having issues with the type definitions. I made these changes locally and it worked. My only concern is I don't have a great idea about backwards compatibility with older versions of the typescript compiler (I'm using 2.7.1 -- the latest stable).

see: https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-class-d-ts.html

@aoberoi
Copy link
Contributor Author

aoberoi commented Feb 3, 2018

Ping @delta62. It looks like you might be the resident TypeScript expert for this project

@aoberoi
Copy link
Contributor Author

aoberoi commented Feb 3, 2018

The failure above seems unrelated, and has something to do with Sauce Labs.

@lpinca
Copy link
Member

lpinca commented Feb 3, 2018

Ignore the failure. Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

@lpinca
Copy link
Member

lpinca commented Feb 3, 2018

cc: @Stubb0rn

@delta62
Copy link
Contributor

delta62 commented Feb 3, 2018

Okay, so it seems there are a few test cases to follow through on here:

  • Use TS 2.7 with the new esModuleInterop flag, importing as a default import
  • Use TS 2.7 with the new esModuleInterop flag, importing as a named import
  • Use TS 2.7 with the new esModuleInterop flag, importing using import = require
  • Use TS 2.6 using a named import
  • Use TS 2.6 using import = require
  • Use any TS version to ensure that EventEmitter is not leaked as a globally defined construct without explicitly importing it

We could also test 2.7 without the esModuleInterop stuff, but I think that's going to be redundant if we test it in 2.6.

@delta62
Copy link
Contributor

delta62 commented Feb 5, 2018

It doesn't look like named imports (import { EventEmitter } from 'eventemitter3') are working with this build.

I can spend some time this week fixing it up, or you can take another stab at it @aoberoi!

@delta62
Copy link
Contributor

delta62 commented Feb 6, 2018

@aoberoi I've created a PR into your feature branch that should clear up the issues.

Add named export for TypeScript
@aoberoi
Copy link
Contributor Author

aoberoi commented Feb 6, 2018

thanks @delta62! just merged, so this PR should inherit the change :)

@lpinca lpinca merged commit 7b2f6d0 into primus:master Feb 7, 2018
@lpinca
Copy link
Member

lpinca commented Feb 7, 2018

@aoberoi @delta62 thank you!

@dcharbonnier
Copy link

dcharbonnier commented Feb 7, 2018

this break my build ;-)

lib/IConnectionDriver.ts(3,44): error TS2304: Cannot find name 'EventEmitter'.
lib/IConnectionDriver.ts(3,44): error TS4022: 'extends' clause of exported interface 'IConnectionDriver' has or is using private name 'EventEmitter'.

The solution is to replace :
import { EventEmitter } from "eventemitter3";

With import * as EventEmitter from "eventemitter3";

@lpinca
Copy link
Member

lpinca commented Feb 7, 2018

Ops, sorry about that. Happy to cut a new release if you know how to fix that :)

@delta62
Copy link
Contributor

delta62 commented Feb 7, 2018

@dcharbonnier Could you include your tsconfig.json so that we can look into fixing this for other people with your setup?

@yahyavi
Copy link

yahyavi commented Feb 14, 2018

I have a similar problem as @dcharbonnier

@delta62
Copy link
Contributor

delta62 commented Feb 14, 2018

Hi @yahyavi. Could you please post your tsconfig.json? I have a test repository that seems to be working for both 2.6 and 2.7, so I'm guessing something specific to the setup you guys are using causes the issue.

@yahyavi
Copy link

yahyavi commented Feb 14, 2018

Sure, here you go:

{
    "compilerOptions": {
        "allowJs": true,
        "module": "commonjs",
        "lib": [
            "dom",
            "es6",
        ],
        "target": "es6",
        "sourceMap": true
    },
    "include": [
        "./src/**/*"
    ]
}

@delta62
Copy link
Contributor

delta62 commented Feb 14, 2018

Thanks a lot! In this particular case, the code is working as I would expect.

// This works
// import * as EventEmitter from 'eventemitter3'

// This works
// import { EventEmitter } from 'eventemitter3'

// This works
// import EventEmitter = require('eventemitter3')

// This works with esModuleInterop = true
// import EventEmitter from 'eventemitter3'

const emitter = new EventEmitter()
emitter.on('some-event', () => console.log('success!'))
emitter.emit('some-event')

But maybe I'm missing something, or you had a different use-case in mind?

edit: added import = example

@yahyavi
Copy link

yahyavi commented Feb 14, 2018

In my case import * as EventEmitter from 'eventemitter3' works but import { EventEmitter } from 'eventemitter3' does not.
Could be a webpack issue?

@delta62
Copy link
Contributor

delta62 commented Feb 14, 2018

Ah, that would make sense. If I recall correctly, WebPack makes use of Babel to bundle the files under the hood, so there could be a configuration difference between WebPack and TypeScript.

@yahyavi
Copy link

yahyavi commented Feb 15, 2018

Yeah, my guess this release broke something with webpack, because before that import { EventEmitter } from 'eventemitter3' was working fine.

@delta62
Copy link
Contributor

delta62 commented Feb 20, 2018

Hmm, I still can't repro this.

I created a webpack / ts-loaderproject, and I can still import in any of the ways mentioned above. I updated my sample project to reflect this (see the webpack branch).

It sounds like you've got a workaround for this, so let's open a new issue if we need to continue debugging!

@yahyavi
Copy link

yahyavi commented Feb 20, 2018

I have a deadline right now. Will try to have a look at the sample project in a couple of weeks.

@shaharmor
Copy link
Contributor

This change breaks extending EventEmitter3:

import { EventEmitter } from 'eventemitter3';


export class MyEmitter extends EventEmitter {
  public constructor() {
    super();
  }
}

results in the following error: Base constructor return type '() => EventEmitter' is not a class or interface type

@delta62
Copy link
Contributor

delta62 commented Apr 13, 2018

Hi @shaharmor!

I put the sample code you have above into my test project and it's compiling ok. I am using eventemitter3@3.0.1 / typescript@2.7.1. Maybe we have different build environments?

If you're still able to repro, let's open a new ticket to track this 🙂

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.

6 participants