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

feature(typescript): add typescript to svgr/core [WIP] #130

Closed
wants to merge 8 commits into from

Conversation

marques-kevin
Copy link
Contributor

@marques-kevin marques-kevin commented Jun 21, 2018

@neoziro Hello !

Really good job for the refacto :)

I have a problem with svgr/cli, I want to run the tests of the index.test.js but I have this error message :


  ● cli › --use-tabs

    Command failed: bin/svgr --use-tabs __fixtures__/one.svg
    /bin/sh: bin/svgr: No such file or directory



  ● cli › --out-dir

    Command failed: bin/svgr --out-dir __fixtures_build__ __fixtures__
    /bin/sh: bin/svgr: No such file or directory



  ● cli › --precision

    Command failed: bin/svgr --precision 10 __fixtures__/one.svg
    /bin/sh: bin/svgr: No such file or directory



  ● cli › --svg-attributes

    Command failed: bin/svgr --svg-attribute focusable=false --svg-attribute hidden=0 __fixtures__/one.svg
    /bin/sh: bin/svgr: No such file or directory

I tried to run yarn build but it not fix this, any idea ?

@marques-kevin
Copy link
Contributor Author

@neoziro No sorry It's good, I did a yarn build and it created a lib directory with tests files, so when I run jest, it tries to exec the tests in the lib directory, so I have to delete it and now it works :)

@gregberge
Copy link
Owner

@marquesdev it looks great! Ping me when it is done (documentation).

@codecov
Copy link

codecov bot commented Jul 4, 2018

Codecov Report

Merging #130 into master will decrease coverage by 1.88%.
The diff coverage is 69.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   88.55%   86.66%   -1.89%     
==========================================
  Files          26       27       +1     
  Lines         428      465      +37     
  Branches      113      127      +14     
==========================================
+ Hits          379      403      +24     
- Misses         37       51      +14     
+ Partials       12       11       -1
Impacted Files Coverage Δ
packages/core/src/config.js 100% <ø> (ø) ⬆️
packages/webpack/src/index.js 94.73% <100%> (ø) ⬆️
packages/rollup/src/index.js 100% <100%> (ø) ⬆️
packages/core/src/plugins/transform.js 100% <100%> (ø) ⬆️
...ages/core/src/templates/reactTypescriptTemplate.js 62.85% <65.62%> (ø)
packages/core/src/plugins/h2x.js 96.29% <0%> (ø) ⬆️
packages/core/src/h2x/svgAttributes.js 76.92% <0%> (ø) ⬆️
packages/core/src/h2x/titleProp.js 75.75% <0%> (ø) ⬆️
packages/core/src/h2x/replaceAttrValues.js 72.72% <0%> (ø) ⬆️
... and 1 more

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 fa09c9b...d62feb6. Read the comment docs.

@@ -119,6 +120,8 @@ async function run() {
}
}

if(config.typescript) config.ext = "ts"
Copy link

@JReinhold JReinhold Jul 9, 2018

Choose a reason for hiding this comment

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

it looks here, like we are forcing the ".ts" extension on typescript parsing which makes perfect sense, however some people prefer using ".tsx" instead. I would recommend something in the line of:

if(config.typescript && !config.ext) config.ext = "ts";

to make sure that the user still manually can override the extension.
(if this is already covered, then i'm sorry about the interruption)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thks for the help :)

} else if (opts.titleProp && opts.ref) {
props = '{svgRef: string, title: string}'
} else if (opts.expandProps) {
props = 'props: any'
Copy link

@JReinhold JReinhold Jul 9, 2018

Choose a reason for hiding this comment

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

I've had great success here, typing the props as SVGAttributes<SVGElement> imported with import React, { SVGAttributes } from 'react'; instead of any type.

Mind though, that you would then need to create a few union types for the cases above, such as {svgRef, ...props}: SVGAttributes<SVGElement> & {svgRef: string}

@JReinhold
Copy link

added a few comments, hope it's okay. 🙂

@gregberge
Copy link
Owner

Also what about using React Native + TypeScript. @JReinhold do you know if it is common to use TypeScript + React Native? Should we support it?

@JReinhold
Copy link

I don't have any numbers, but my gut feeling says that the React Native+TypeScript combo has almost the same usage as the React+TypeScript combo, since the benefits and hurdles of TS are the exact same across React/React Native.

So yes, I would suggest to support RN as well. Could be in a second PR though.

@gregberge
Copy link
Owner

Yes but it is pretty confusing for users that if I use --native --typescript I got either not a native output or not a TypeScript output. So I would prefer to have it in this PR.

@JReinhold
Copy link

I see. Agree!

@gregberge
Copy link
Owner

Outdated, we restart from scratch on it.

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.

3 participants