[styled-components] Move React Native-dependent types to separate package #49914
Conversation
typescript-bot
added
New Definition
|
@Methuselah96 Thank you for submitting this PR! This is a live comment which I will keep updated. 2 packages in this PR
Code ReviewsThis PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 49914,
"author": "Methuselah96",
"headCommitAbbrOid": "48542fd",
"headCommitOid": "48542fd96a519ae9ae79d1c1dd70c771a6f13e83",
"lastPushDate": "2020-12-18T16:04:32.000Z",
"lastActivityDate": "2020-12-18T16:42:46.000Z",
"maintainerBlessed": false,
"hasMergeConflict": false,
"isFirstContribution": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "styled-components-react-native",
"kind": "add",
"files": [
{
"path": "types/styled-components-react-native/index.d.ts",
"kind": "definition"
},
{
"path": "types/styled-components-react-native/styled-components-react-native-tests.tsx",
"kind": "test"
},
{
"path": "types/styled-components-react-native/tsconfig.json",
"kind": "package-meta",
"suspect": "not [the required form](https://github.com/DefinitelyTyped/DefinitelyTyped#tsconfigjson)"
},
{
"path": "types/styled-components-react-native/tslint.json",
"kind": "package-meta",
"suspect": "not [the required form](https://github.com/DefinitelyTyped/DefinitelyTyped#linter-tslintjson)"
}
],
"owners": [],
"addedOwners": [
"Methuselah96"
],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "styled-components",
"kind": "edit",
"files": [
{
"path": "types/styled-components/native.d.ts",
"kind": "definition"
},
{
"path": "types/styled-components/ts3.6/native.d.ts",
"kind": "definition"
},
{
"path": "types/styled-components/ts3.6/test/native.tsx",
"kind": "test"
},
{
"path": "types/styled-components/ts3.6/tsconfig.json",
"kind": "package-meta",
"suspect": "not [the required form](https://github.com/DefinitelyTyped/DefinitelyTyped#tsconfigjson) and not moving towards it"
},
{
"path": "types/styled-components/tsconfig.json",
"kind": "package-meta",
"suspect": "not [the required form](https://github.com/DefinitelyTyped/DefinitelyTyped#tsconfigjson) and not moving towards it"
}
],
"owners": [
"Igorbek",
"Igmat",
"lavoaster",
"Jessidhia",
"jkillian",
"eps1lon",
"flavordaaave",
"wagerfield",
"Lazyuki",
"mgoszcz2",
"danilofuchs"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "eps1lon",
"date": "2020-12-18T16:42:46.000Z",
"isMaintainer": false
},
{
"type": "stale",
"reviewer": "alloy",
"date": "2020-12-16T12:12:35.000Z",
"abbrOid": "aa7deb3"
},
{
"type": "stale",
"reviewer": "paulmelnikow",
"date": "2020-12-15T17:39:32.000Z",
"abbrOid": "aa7deb3"
}
],
"ciResult": "pass"
} |
|
🔔 @Igorbek @Igmat @lavoaster @Jessidhia @jkillian @eps1lon @flavordaaave @wagerfield @Lazyuki @mgoszcz2 @danilofuchs — please review this PR in the next few days. Be sure to explicitly select |
typescript-bot
moved this from Waiting for Code Reviews
to Needs Author Action
in New Pull Request Status Board
|
@Methuselah96 The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@Methuselah96 The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? styled-components-react-native/v5.1These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to styled-components-react-native with its source on master. Comparison details for styled-components-react-native/5.1 📊
styled-components/v5.1
Looks like there were a couple significant differences—take a look at mean duration for getting completions at a position and mean duration for getting quick info at a position to make sure everything looks ok. |
typescript-bot
added
the
Perf: Worse
|
Build failing due to microsoft/TypeScript#41790, but should be good besides that. |
|
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. styled-components (unpkg)was missing the following properties:
|
typescript-bot
moved this from Needs Author Action
to Needs Maintainer Action
in New Pull Request Status Board
|
Updated numbers for you here from eb300d1. styled-components-react-native/v5.1These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to styled-components-react-native with its source on master. Comparison details for styled-components-react-native/5.1 📊
styled-components/v5.1Comparison details for styled-components/5.1 📊
Looks like there were a couple significant differences—take a look at mean duration for getting completions at a position and mean duration for getting quick info at a position to make sure everything looks ok. |
|
👋 - I've done my digging. TLDR: I think we should merge this. I have a sample repo with before and after examples for both node and react native projects here with the README being a summary of how they act out of the box. What this PR proposes is that we remove the Upsides
Downsides
A React Native project which does not have Trade-offsGiven the size differences in the amount of developers working on the web vs react native, I think having SC broken for the web in favour of RN support being 1st class isn't a great trade-off. There's a lot of creative answers to the issue, but I think this is the best option we have right now. With the cost being that people need to learn to get the new dependency for React Native, which I think is pretty reasonable. IMO, I think these trade-offs can be mitigated by improving the Styled Components docs in the TypeScript section. The docs can mention TS + RN specifically as needing this extra dep, and I think that should be reasonably harmless. Ideally a FAQ entry can be added around this error message: I'm going to go ping a few threads asking for feedback on this PR as a solid potential solution. Then once more folks have given input we can make a call on whether to merge or not 👍🏻 - thanks for getting the ball rolling @Methuselah96
|
typescript-bot
added
the
Unreviewed
|
Re-ping @Igorbek, @Igmat, @lavoaster, @Jessidhia, @jkillian, @eps1lon, @flavordaaave, @wagerfield, @Lazyuki, @mgoszcz2, @danilofuchs: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
|
This is more than needed. |
There was a problem hiding this comment.
#49914 (comment) summarizes this perfectly. @orta Thanks looking into the impact this change has 👍
typescript-bot
added
the
Owner Approved
|
This all looks good. Just a question. Could we somehow include an error, if someone uses Basically there's |
|
Updated numbers for you here from 86733a3. styled-components-react-native/v5.1These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to styled-components-react-native with its source on master. Comparison details for styled-components-react-native/5.1 📊
styled-components/v5.1Comparison details for styled-components/5.1 📊
Looks like there were a couple significant differences—take a look at mean duration for getting completions at a position and mean duration for getting quick info at a position to make sure everything looks ok. |
|
Alright, we are good to go - thanks for the rebase @Methuselah96 ! |
|
I just published |
|
I just published |
elibarzilay
moved this from Needs Maintainer Action
to Recently Merged
in New Pull Request Status Board
|
I'm probably missing something simple but how do you actually use this? I've tried a bunch of variations on installing the new Native types lib, the old one, only one of them, both of them, setting a path in tsconfig... If I just install both of them then when I try to import
I'm just trying to make a |
|
Ok, here's what seems to be working:
If this is the intended way it seems it would be worthwhile to document in the npm readme. |
|
Is it possible that you have the |
|
@Methuselah96 Thanks for the help - yes, we specify Seems worth putting in readme.md to me since I haven't run into this in other libs in a fair amount of experience, + using paths is common as far as I know (e.g. for Jest as we do). |
|
Apologies for the confusion. It would be great if you could make a PR to add documentation for this case to the TypeScript section on the styled-components website. I think it's more common than not that people aren't specifying the |
If this isn't done then the types won't be found, if `types` is already set. Per author @Methuselah96: DefinitelyTyped/DefinitelyTyped#49914 (comment)
If this isn't done then the types won't be found, if `types` is already set. Per author @Methuselah96: DefinitelyTyped/DefinitelyTyped#49914 (comment)
If this isn't done then the types won't be found, if `types` is already set. Per author @Methuselah96: DefinitelyTyped/DefinitelyTyped#49914 (comment)
No problem, thanks again for the help and I submitted a PR. |
|
Thank you so much @lukewlms and @Methuselah96 -- without finding this issue, I would never have solved the problem of the types not being found. |
|
It seems like @types/styled-components to 5.1.7 has broken this fix. I have @types/styled-components-react-native installed as well and I cant seem to find any workarounds to get this to work other than setting "noImplicitAny" to false |
|
@davisk4rpi - make sure you're not using any other definitions in 'typeSrc' or 'types' - if you are, it breaks. |
|
Just to clarify, you're allowed to have other definitions in |
|
Yes. Sorry. I should have said if it's a not empty array you should added the types to the array. |
|
@ortonomy @Methuselah96 Thanks, That was it. didnt notice the types field in my config the first time through |
Resolves #33311
Resolves #33015
The problem
Both of the above issues have been open for over a year and a half and the first one has more up-votes than any other issue on DefinitelyTyped. The current solution on these issues is to set the
types: []intsconfig.jsonto pull in the appropriate types, which means@types/styled-componentsdoesn't work out-of-the-box unlike most other@typespackages. This harms not only the reputation of TypeScript, but also the reputation ofstyled-componentsand causes unnecessary friction in order to use these types.The solution
Move the
styled-components/nativetypes out of@types/styled-componentsand into its own package (@types/styled-components-react-native). If someone wants to usestyled-components/native, all they need to do is install@types/styled-components-react-nativeand it will automatically augment thestyled-componentstypes (since TypeScript automatically includes any installed@typespackages).This makes it so that the main export (i.e.,
import styled from 'styled-components') works out-of-the-box (which is the most common usage). And in order to getstyled-components/nativeall you need to do is install the other@typespackage, which is still easier than having to settypes: []intsconfig.json.Note: this is still just a temporary workaround, but it is still better than the current situation.
Please fill in this template.
npm test <package to test>.If adding a new definition:
.d.tsfiles generated via--declarationdts-gen --dt, not by basing it on an existing project.tslint.jsonshould be present and it shouldn't have any additional or disabling of rules. Just content as{ "extends": "dtslint/dt.json" }. If for reason the some rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.tsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.