Skip to content
This repository has been archived by the owner on Aug 28, 2022. It is now read-only.

added typescript declarations #67

Merged
merged 6 commits into from
Sep 27, 2017

Conversation

kaoDev
Copy link
Contributor

@kaoDev kaoDev commented Sep 3, 2017

Hey,
I used glamorous-native in a Typescript project and wanted to contribute my declaration files so that other Typescript projects can benefit.

Greetings
Kalle

@codecov-io
Copy link

codecov-io commented Sep 3, 2017

Codecov Report

Merging #67 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #67   +/-   ##
======================================
  Coverage    92.3%   92.3%           
======================================
  Files          11      11           
  Lines         143     143           
  Branches       40      40           
======================================
  Hits          132     132           
  Misses          7       7           
  Partials        4       4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c74dbc...d0fc793. Read the comment docs.

@atticoos
Copy link
Contributor

atticoos commented Sep 8, 2017

Thanks for this contribution! @luke-john has done a lot with Typescript over in paypal/glamorous, would you mind taking a look? I'm probably not the best to review this

@kaoDev
Copy link
Contributor Author

kaoDev commented Sep 8, 2017

yes I think @luke-john is just the right person :), I used the paypal/glamorous typings as a basis to create thos for native

Copy link

@luke-john luke-john left a comment

Choose a reason for hiding this comment

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

These are looking good to me.

It would be nice to have at least a simple test file which has a few of the common usage patterns covered.

Even if the file isn't part of any of the official tests, it should make it easier for additional maintainers/contributors to see what the intention is behind some of the typings.

ExternalProps &
Partial<DefaultProps> &
Omit<Props, 'theme'> &
Props,

Choose a reason for hiding this comment

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

This Props was a bug in the glamorous typings which has been fixed.

Choose a reason for hiding this comment

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

Also this is missing ElementProps (also recently fixed in glamorous)

@luke-john
Copy link

luke-john commented Sep 10, 2017

This is awesome, really happy to see glamorous-native get typings as well 💖.

I wonder if we could share some of the typings across glamorous and glamorous-native 🤔 , that way both projects would get to take advantage of any improvements and fixes to the shared parts. Reading through these typings glamorous has already received a couple of improvements since these were branched.

The easiest way would be to add glamorous as a dependency and these typings could import the common parts from there, but that's a pretty massive dependency just for the typings.

Alternatively, and probably the better solution, would be to re-publishing the typings seperate to glamorous.

I'm not likely to get time to look into getting this setup in the next week though, so I'd proceed with landing these typings as is 👍 .

@kaoDev
Copy link
Contributor Author

kaoDev commented Sep 10, 2017

Hey Luke thank you for the feedback, would be really cool to have a common base for the typings. I will try to get some time to think about this, but the next weeks I am still pretty stuffed with work

@esamattis
Copy link
Contributor

Hi, I'd like to test these typings but I'm having hard time figuring out how to add these to my project so that TS would pick them up for glamorous-native imports.

I tried creating typings/glamorous-native with the type definitions files and adding tsconfig.json entry: "typeRoots": ["./node_modules/@types", "./typings"] but no luck so far.

Could someone point me to right direction? Thanks.

@atticoos
Copy link
Contributor

Aiming to merge this up! @kaoDev any insight on the above if any changes are needed in this PR?

@atticoos atticoos self-assigned this Sep 19, 2017
@luke-john
Copy link

Unfortunately the way typescript handles definitions under typeRoots is not consistant with bundled typings (or packaged types ¯_(ツ)_/¯). The easiest way to give them a go for testing would be to add them into the glamorous-native folder in node_modules and update the package.json to contain the following property.

"types": "./typings/glamorous.d.ts",

@kaoDev
Copy link
Contributor Author

kaoDev commented Sep 19, 2017

hi @epeli,
TypeScript looks for the types entry in the package.json, in this PR it is added to point to "./typings/glamorous.d.ts" as an entrypoint. So when it is merged it should work out of the box.
I just searched for a good tutorial to configure typescript to handle custom typings for untyped node-modules but unfortunately wasn't successful. So a very short writeup, potentially incomplete:

  1. add a folder "declarations" to your project
  2. add a folder named like the module you are writing/testing declarations for
  3. inside the module folder add the declaration files, if it is an index.d.ts TypeScript will match it automatticaly otherwise add a package.json with the "types" property pointing to the typings main file
  4. add the baseUrl and paths to the tsconfig:
    "baseUrl": ".",
    "paths": {
          "*": [
                "declarations/*"
          ]
    }
  5. make sure "moduleResolution": "node" is set in the tsconfig file

I hope this is helpful and you can test the typings 😄

@luke-john was faster, that's another possibility

@esamattis
Copy link
Contributor

I just searched for a good tutorial to configure typescript to handle custom typings for untyped node-modules but unfortunately wasn't successful.

Yeah I tried that too with no luck but your writeup seems good, thanks! I'll post here if I find something strange.

Copy link
Contributor

@atticoos atticoos left a comment

Choose a reason for hiding this comment

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

Thanks for this, sorry it took a little while to get in!

@atticoos atticoos merged commit c438939 into robinpowered:master Sep 27, 2017
@atticoos atticoos mentioned this pull request Sep 27, 2017
@esamattis
Copy link
Contributor

No worries. Thanks for this!

Didn't find anything unusual during my testing btw.

@esamattis
Copy link
Contributor

esamattis commented Oct 3, 2017

Now I did. There is a definition for withProps but that does not seem to actually exists glamorous-native but it does on the web based glamorous.

@kaoDev
Copy link
Contributor Author

kaoDev commented Oct 3, 2017

ah, thanks for the report. I will try to find some time today to make a PR for this bad copy pasta

@kaoDev kaoDev deleted the feature/typescript branch October 4, 2017 21:55
@kaoDev
Copy link
Contributor Author

kaoDev commented Oct 4, 2017

#77

@esamattis
Copy link
Contributor

esamattis commented Oct 11, 2017

How about usage of propsAreStyleOverrides? At least at first glance it seems to be fundamentally incompatible with static typing as it changes the type by runtime mutation.

I'm trying make an animated image

const Character = g(Animated.Image)({
    height: 270,
    resizeMode: "contain",
    width: "100%",
});
Character.propsAreStyleOverrides = true; // type error
Property 'propsAreStyleOverrides' does not exist on type 'GlamorousComponent<object & {}, object>'.
<Character
  source={imageSource}
  top={animatedValue} // type error
Type '{ source: any; top: Value; }' has no properties in common with type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<ExtraGlamorousProps & object, ComponentS...'.

I think this is workaroundable with some clever type assertion but I didn't figure out that yet.

But in the end I'd like to see some other API for this which would be better fit for typed languages (ts&flow).

@luke-john
Copy link

@epeli, assuming glamorous-native supports propsAreCssOverrides, you should be able to achieve it like follows currently.

I've added a comment to the issue created by @ajwhite about what would be needed to bring that typing support natively to glamorous-native.

const propsAreCssOverrides = {propsAreCssOverrides: true as false}
const Character = g(Animated.Image, propsAreCssOverrides)({
    height: 270,
    resizeMode: "contain",
    width: "100%",
});

@esamattis
Copy link
Contributor

Nope

Type '{ source: any; top: Value; }' has no properties in common with type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<ExtraGlamorousProps & object, ComponentS...'.

The Value is Animated.Value btw.

@slorber
Copy link
Contributor

slorber commented Jun 12, 2018

Hey all.

I'm not using typescript, but it seems typescript definitions are useful for my Intellij/Webstorm IDE in many cases where it can't infer proper types.

In this lib I constantly get failing imports on Glamorous-native, does anyone know how to add support for it in Intellij?

image

I tried to download the typings but for this lib it does not appear in the download list (probably because they are not published?)
https://stackoverflow.com/questions/35741847/cannot-resolve-symbol-when-using-es6-import-syntax

Also tried to setup the bindings manually but I failed (not sure exactly how to do that)

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

Successfully merging this pull request may close these issues.

None yet

6 participants