Add TailwindCSS type definitions #50921
Conversation
|
@dolanmiu Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode 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": 50921,
"author": "dolanmiu",
"headCommitOid": "3b1d1f5606b2c4285967613d183139d4d083226f",
"lastPushDate": "2021-03-01T22:40:14.000Z",
"lastActivityDate": "2021-03-01T22:57:05.000Z",
"maintainerBlessed": false,
"hasMergeConflict": false,
"isFirstContribution": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "tailwindcss",
"kind": "add",
"files": [
{
"path": "types/tailwindcss/colors.d.ts",
"kind": "definition"
},
{
"path": "types/tailwindcss/defaultTheme.d.ts",
"kind": "definition"
},
{
"path": "types/tailwindcss/index.d.ts",
"kind": "definition"
},
{
"path": "types/tailwindcss/resolveConfig.d.ts",
"kind": "definition"
},
{
"path": "types/tailwindcss/tailwind-config.d.ts",
"kind": "definition"
},
{
"path": "types/tailwindcss/tailwindcss-tests.ts",
"kind": "test"
},
{
"path": "types/tailwindcss/tsconfig.json",
"kind": "package-meta-ok"
},
{
"path": "types/tailwindcss/tslint.json",
"kind": "package-meta-ok"
}
],
"owners": [],
"addedOwners": [
"dolanmiu"
],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "stale",
"reviewer": "amcasey",
"date": "2021-03-01T22:33:15.000Z",
"abbrOid": "8d91a82"
},
{
"type": "stale",
"reviewer": "themagickoala",
"date": "2021-02-18T23:08:45.000Z",
"abbrOid": "8d91a82"
},
{
"type": "stale",
"reviewer": "aivenkimmob",
"date": "2021-02-18T12:19:24.000Z",
"abbrOid": "8d91a82"
},
{
"type": "stale",
"reviewer": "peterblazejewicz",
"date": "2021-02-14T19:50:05.000Z",
"abbrOid": "8d91a82"
},
{
"type": "stale",
"reviewer": "sheetalkamat",
"date": "2021-01-29T22:55:24.000Z",
"abbrOid": "2049749"
}
],
"ciResult": "pass"
} |
|
🔔 @dolanmiu — you're the only owner, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...) |
typescript-bot
added
New Definition
|
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. tailwindcss (unpkg)was missing the following properties:
|
|
👋 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? These 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 tailwindcss with its source on master. Comparison details 📊
|
typescript-bot
added
the
Revision needed
|
@dolanmiu One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
This is now done |
typescript-bot
removed
Check Config
|
@sheetalkamat Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
|
@sheetalkamat Could you take a look again please? |
| @@ -0,0 +1,1186 @@ | |||
| export type Variant = | |||
There was a problem hiding this comment.
This appears to be a types-only file without a corresponding module in the underlying library. Am I reading that correctly? Is the goal just to prevent consumers from accidentally importing the types? If not, why not just include them in index.d.ts? Possible issues: if a consumer does import this file, will the import still be present at runtime? What happens if the underlying library adds a module with this name (unlikely, since it doesn't seem to follow their naming conventions)?
There was a problem hiding this comment.
Separation of concerns
tailwind-config.d.ts is really big (way over 1000+ lines long), and I did not want to pollute index.d.ts
This library does not export anything of the sort, I had to manually infer this from the documentation.
Similar to how in @types/webpack type definitions, there is a Configuration interface which is no where to be found in webpack itself. This is my goal
There was a problem hiding this comment.
Sorry, I'm not sure I understand. What concerns are you separating? It seems like tailwind-config.d.ts contains the types that are necessary for the declarations in index.d.ts. If you don't want consumers to see the helper types, just don't export them.
I don't follow your point about webpack. There's nothing wrong with adding types not found in the underlying library - that's the point of DefinitelyTyped. My concern is with adding a package not found in the underlying library. Does webpack do that?
There was a problem hiding this comment.
This type definition has zero dependencies, so surely this would not be a problem?
Ok, I could move tailwind-config.d.ts into index.d.ts if that is better? what do you think?
There was a problem hiding this comment.
I'm not sure what you mean by "zero dependencies". If you mean that it doesn't depend on anything, I don't think that's relevant to the file organization. If you mean that nothing depends on it, I think that's only because it hasn't been merged yet - it seems to be quite a popular package.
Yes, as @peterblazejewicz and I have explained, having modules that don't exist in the underlying library makes it more likely that a user will consume it incorrectly and have problems at runtime.
types/tailwindcss/resolveConfig.d.ts
Outdated
| @@ -0,0 +1,5 @@ | |||
| import { TailwindConfig } from './tailwind-config'; | |||
There was a problem hiding this comment.
there is no './tailwind-config.jsin the source, why this cannot belib/util/resolveConfig` import?
There was a problem hiding this comment.
It can't be lib/util/resolveConfig because it has to be:
import resolveConfig from 'tailwindcss/resolveConfig'Is this what you mean?
Source:
https://tailwindcss.com/docs/configuration#referencing-in-java-script
There was a problem hiding this comment.
This is how to consume the package IMO, not how it's written:
https://github.com/tailwindlabs/tailwindcss/blob/master/resolveConfig.js#L4
https://unpkg.com/tailwindcss@2.0.2/resolveConfig.js
this methods returns outcome of calling the resolvveConfigObjects:
const resolveConfigObjects = require('./lib/util/resolveConfig').defaultyou can of course skip 'lib/util/resolveConfig' details and just write shared types somewhere. But I'd not create some artificial file for those, just put them into 'index.d.ts' as shared interfaces (types).
Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com>
Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com>
There was a problem hiding this comment.
Can you please address @aivenkimmob's question? The types shouldn't preclude common usage patterns.
typescript-bot
added
Revision needed
|
@dolanmiu One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
done |
|
Could someone merge this in? Everything is addressed |
Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com>
typescript-bot
removed
the
Revision needed
|
@amcasey I had to revert the |
|
@dolanmiu The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
@amcasey, @themagickoala, @aivenkimmob, @peterblazejewicz, @sheetalkamat Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
The error is correct - you need a default export to use a default import. I'm confused because the underlying code does not appear to have a default export. However, as you point out, the docs suggest doing it this way, so I'm fine with reverting the export. |
|
I just published |
Please fill in this template.
npm test <package to test>.Select one of these and delete the others:
If adding a new definition:
.d.tsfiles generated via--declarationdts-gen --dt, not by basing it on an existing project.tslint.jsonshould contain{ "extends": "dtslint/dt.json" }, and no additional rules.tsconfig.jsonshould havenoImplicitAny,noImplicitThis,strictNullChecks, andstrictFunctionTypesset totrue.Documentation for why:
https://tailwindcss.com/docs/configuration#referencing-in-java-script
See "Referencing in JavaScript"