node-strava / node-strava-v3

API wrapper for Strava's v3 API, in Node
MIT License
356 stars 109 forks source link

Error in typings #100

Closed ntziolis closed 3 years ago

ntziolis commented 4 years ago

@nik0kin There seems to be an issue with the typings:

The following is allowed by the typings but results in strava being undefined during runtime

import { strava } from 'strava-v3' // 

So we tried using the default import and now strava is defined during runtime

import * as strava from 'strava-v3' // this results in strava being defined during runtime

But the issue with that:

The typings suggest that strava does not use default exports, or at least in addition also exports the strava constant. Howveer that does not seem to be the case:

Looking at the js there seems to be only a single default export made, hence import { strava } syntax does not work: https://github.com/UnbounDev/node-strava-v3/blob/a1bfcb3664e6ef4921c8c91b7a3cd295185ce70f/index.d.ts#L158 Should be:

declare const strava : Strava;

And: https://github.com/UnbounDev/node-strava-v3/blob/a1bfcb3664e6ef4921c8c91b7a3cd295185ce70f/index.d.ts#L160 Should be

export = strava;

This is only a partial fix as this excludes all interfaces from being exported. in order for this to work add the following line at the end:

export as namespace strava;

Which allows users to access the interfaces as follows:

const routes: strava.ActivitiesRoutes

Inspiration taken from moment: https://github.com/moment/moment/blob/63f3d52945bc773925b862c61ee7a322d4a33308/ts3.1-typings/moment.d.ts#L784

markstos commented 4 years ago

Please submit this as a PR.

vtimbuc commented 4 years ago

@ntziolis @markstos I'm getting an error since this update (v2.0.8):

An export assignment cannot be used in a module with other exported elements.

error

markstos commented 4 years ago

@ntziolis Please work with @vtimbuc to resolve this. I'm not currently using the TypeScript bindings and trusted that that would change was correct. It could helpl if you both mentioned the version of Node.js and TypeScript that you are using.

nik0kin commented 4 years ago

I see the same error as @vtimbuc (typescript version 3.7.2)

ntziolis commented 4 years ago

Will look into this later today. Using typescript 4.0.3. @vtimbuc & @nik0kin Did the bidnings work for you before the update? especially exporting strava itself? See my error description above.

Or where you only using the different input / output types for activities etc? This also worked for me but importing strava itself did not.

nik0kin commented 4 years ago

Yes, it works with version 2.0.7 using ts-node or tsc compiling to commonjs. tsconfig.json Here's how I use strava-v3

Did you try importing like: import strava from 'strava-v3';

ntziolis commented 4 years ago

First off apologies for breaking stuff for you guys. After looking into this a bit further it seems the typing I provided are only supported by later typescript versions.

Using implicit default importing is disabled by policy (as is the default by now) in our projects, hence we could no use: import strava from 'strava-v3'

After reverting to the previous typings + some fiddling we found the following works well: import { default as strava, Strava} from 'strava-v3';

So for the time being I'm asking to revert back to the previous typings. By simply reverting the commit made by the PR.

That said there is an issue with the typing, since other imports are also possible but produce runtime errors like: import * as strava from 'strava-v3'

I suggest calling out the recommended way of importing this into typescript projects on the npm page. It would be helpful to provide an sample that works across typescript versions and does not require changing compiler settings.

@nik0kin Can you check in your setup if the following provides correct typings + has the strava object populated during runtime: import { default as strava, Strava} from 'strava-v3';

ntziolis commented 4 years ago

I prepared a PR https://github.com/UnbounDev/node-strava-v3/pull/102 that:

The second step is key to prevent people from importing this as we did:

import { strava, Strava} from 'strava-v3'

const test: Strava = strava;

This was previously possible previously, but didn't work since the js does not provide a named export strava. Hence it failed during runtime.

@nik0kin Would you be so kind and verify that this doesn't break anything on your end? Easiest way is to go into your node_moduls folder and replace the last lines in the strava-v3 index.dts

nik0kin commented 4 years ago

importing the default (either way) from strava-v3 works for me with:

declare const strava: Strava;
export default strava;

(There has to a variable named strava to be able to export it as default)

markstos commented 4 years ago

Fixed in v2.0.9, just published.