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

Typescript definitions don't work #72

Closed
dwickern opened this issue Sep 11, 2017 · 18 comments
Closed

Typescript definitions don't work #72

dwickern opened this issue Sep 11, 2017 · 18 comments
Labels

Comments

@dwickern
Copy link

If you import fetch from 'fetch', typescript won't resolve it.
If you import fetch from 'ember-fetch', typescript will resolve it, but you'll have a runtime error.

When you import fetch, typescript looks in node_modules/fetch. It won't scan the whole node_modules directory for anyone exporting a fetch module. You can work around this with an explicit path mapping in tsconfig.json, but it's not conventional.

Maybe this package could export ember-fetch, and eventually deprecate fetch?

/cc @toranb @chriskrycho

@chriskrycho
Copy link
Contributor

Ugh. I was hoping that fetch would be an actual npm package, but it's whatwg-fetch, which isn't even a little bit better than ember-fetch. I agree that the best option here is exporting from ember-fetch instead, since the real npm fetch package is something totally different.

Your alternative does bring up an interesting point I'd like to think about in other packages as makes sense: if there's a good reason to, we can presumably teach people how to update the tsconfig.json as part of the addon installation process. That should certainly be a last-resort option, though.

@toranb
Copy link
Contributor

toranb commented Sep 12, 2017

I was actually curious about the (most) correct path forward myself and thought it might be worth asking @DanielRosenwasser to chime in :)

warning and type defs are below for quick reference

A default export can only be used in an ECMAScript-style module.

export = fetch;

declare namespace fetch {
  export default function fetch(input: RequestInfo, init?: RequestInit): Promise<Response>;
}

This is how we import it in our ember applications

import fetch from 'fetch';

@DanielRosenwasser
Copy link

A default export can only be used in an ECMAScript-style module.

Right now by default, TypeScript assumes the mapping between ES modules and CJS modules to be 1:1 (the namespace record and the imported object are treated the same). To get around that, you can use --allowSyntheticDefaultImports.

You can work around this with an explicit path mapping in tsconfig.json, but it's not conventional.

An alternative idea is to add

    "fetch": "ember-cli/ember-fetch"

which will get the current state of ember-fetch from this GitHub repo as a dependency

@toranb
Copy link
Contributor

toranb commented Sep 15, 2017

@DanielRosenwasser thanks for the detailed reply! Reading this it would appear the synthetic option is most appropriate. Any downside to using this option generally speaking?

@toranb
Copy link
Contributor

toranb commented Nov 22, 2017

@chriskrycho @dwickern I just upgraded to typescript v2.6 and this doesn't seem like an issue (anymore?). Here is a repo you can build to confirm it works/ or doesn't for reference

https://github.com/toranb/ember-redux-yelp/commits/branches/seamlessAndTypeScript

Is anyone else having issues w/ TypeScript v2.4/2.5 or 2.6 ?

@theseyi
Copy link

theseyi commented Nov 28, 2017

Still have the same issue with ts2.6 without using a parallel to your solution above, vis-a-viz an ambient module declaration @toranb

@mike-north mike-north added the bug label Dec 20, 2017
@lstrzebinczyk
Copy link

We seem to still have this issue with typescript 2.6.2.

@chriskrycho
Copy link
Contributor

I worked around it long since by doing "fetch": ["node_modules/ember-fetch/index.d.ts"], in my tsconfig.json, so at present that's the thing to do.

I don't agree with @toranb that the synthetic option is the most appropriate. I think it's actually more correct to have the export and import behave correctly at the package level, and then have the types map to that.

@lstrzebinczyk
Copy link

I worked around it by including the types in my own code, but It should probably "just work" when including the package.

@DanielRosenwasser
Copy link

DanielRosenwasser commented Feb 13, 2018

I still kind of think that adding a package.json devDependency of

    "fetch": "ember-cli/ember-fetch"

might be the easiest thing.

@toranb
Copy link
Contributor

toranb commented Feb 17, 2018

Just a quick note here - happy to pass the torch to others who legit know TypeScript :)

When I originally wrote these typedefs I was heads down with TypeScript but these days I don't actually use it (yet) and as a result don't know what is best for library/addon authors. If what @chriskrycho is suggesting seems like the best path forward please feel free to make a move without my involvement. I honestly just want to spin up an ember app (with TypeScript) at some point and get no errors when I turn on strict mode (using libraries like ember-fetch)

@nlfurniss
Copy link
Collaborator

Believe this issue should be solved now that TS support has been added

@theseyi
Copy link

theseyi commented Mar 3, 2019

I believe @xg-wang ‘s PR has been merged but not released. So not sure that it has been “added”. I still had this issue this week, Friday, running the latest released version of ember-fetch.

@theseyi
Copy link

theseyi commented Mar 3, 2019

@nlfurniss

@xg-wang
Copy link
Member

xg-wang commented Mar 3, 2019

Sorry ember-fetch test is passing, but submitting to npm blocked by typed-ember/ember-cli-typescript#566, open PR DefinitelyTyped/DefinitelyTyped#33518

@chriskrycho
Copy link
Contributor

chriskrycho commented Mar 3, 2019 via email

@xg-wang
Copy link
Member

xg-wang commented Mar 12, 2019

image

@theseyi
Copy link

theseyi commented Mar 12, 2019

Always a nice feeling to get rid of workarounds :)

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

No branches or pull requests

9 participants