Reexport @theme-ui/components from theme-ui #41915

hasparus · 2020-01-28T16:29:42Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://theme-ui.com/migrating/, https://github.com/system-ui/theme-ui/blob/master/packages/theme-ui/src/index.js
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
If you are making substantial changes, consider adding a tslint.json containing { "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.

Linked to system-ui/theme-ui#121.

typescript-bot · 2020-01-28T17:22:34Z

@hasparus Thank you for submitting this PR!

🔔 @erikdstock @ifiokjr @sbardian @pomber @wKovacs64 @prateekkathal - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot · 2020-01-28T17:22:35Z

@hasparus The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-01-28T18:00:17Z

👋 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?

theme-ui/v*

Comparison details for theme-ui/v* 📊

	master	#41915	diff
Batch compilation
Memory usage (MiB)	134.2	154.1	+14.8%
Type count	26561	33525	+26%
Assignability cache size	42361	50856	+20%

Language service
Samples taken	115	115	0%
Identifiers in tests	115	115	0%
`getCompletionsAtPosition`
Mean duration (ms)	783.9	826.0	+5.4%
Mean CV	6.7%	7.1%
Worst duration (ms)	1071.1	1175.0	+9.7%
Worst identifier	buttonStyles	background
`getQuickInfoAtPosition`
Mean duration (ms)	773.7	819.6	+5.9%
Mean CV	7.2%	8.2%	+12.9%
Worst duration (ms)	933.2	1049.5	+12.5%
Worst identifier	sx	sx

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

theme-ui__components/v*

Comparison details for theme-ui__components/v* 📊

	master	#41915	diff
Batch compilation
Memory usage (MiB)	207.1	193.8	-6.4%
Type count	38128	38123	0%
Assignability cache size	82663	82663	0%

Language service
Samples taken	351	351	0%
Identifiers in tests	351	351	0%
`getCompletionsAtPosition`
Mean duration (ms)	1231.3	1215.9	-1.3%
Mean CV	6.7%	6.8%
Worst duration (ms)	2136.0	2239.4	+4.8%
Worst identifier	defaultValue	onChange
`getQuickInfoAtPosition`
Mean duration (ms)	1151.5	1135.4	-1.4%
Mean CV	6.4%	7.0%	+9.6%
Worst duration (ms)	2045.7	1968.3	-3.8%
Worst identifier	onChange	Field

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

prateekkathal · 2020-01-28T20:19:40Z

@hasparus I don't think it is a good idea to introduce breaking changes. Maybe we should keep the old keys/properties to support older versions. Please add the below as a comment.

DEPRACATED: Not supported after theme-ui:^0.3.*

Also, I'd suggest adding tests for the newer versions separately rather than changing the old ones.

hasparus · 2020-01-28T20:46:28Z

Maybe we should keep the old keys/properties to support older versions. Please add the below as a comment.

I get the point. It leads to another problem though.
Comments appear in tooltips and that's just it. No errors. So we can update theme-ui to 0.3 without seeing the changelog. Maybe dependabot or greenkeeper made the PR.
We merge the PR, continous deployment runs and our default color mode changed, because initialColorMode is not handled by theme-ui anymore. This is a bug at runtime.

@types packages are versioned. I believe we should match breaking changes of theme-ui (best effort) to help users avoid bugs in runtime. This PR won't break old versions of @types/theme-ui.

I'd find deprecation comment fitting, if theme-ui added a deprecation warning at runtime. There is no warning, thus, this is a breaking change, and we can either help the user deal with it by removing initialColorMode or don't.

Connected issues: #7719 #25677

hasparus · 2020-01-28T21:37:12Z

We needed to upgrade required version to 3.5 because @types/theme-ui__components explodes on TS3.4 (due to "as" prop).

prateekkathal · 2020-01-29T16:16:44Z

types/theme-ui/index.d.ts

 import { Interpolation, SerializedStyles } from '@emotion/serialize';
+export * from '@theme-ui/components';


I am not sure why this belongs here. The goal is to only make types exportable, not components by itself.

Take a look at https://github.com/system-ui/theme-ui/blob/b0257f0aa3dda6e65482552780fda715783205c3/packages/theme-ui/src/index.js#L18

This is the title of my pull request.
ThemeUI 0.3 reexports @theme-ui/components. We want to provide typings for these reexported components, so we can have typechecking and autoimports while using theme-ui.

@hasparus It seems I completely overlooked the fact that a types/theme-ui components was released by you.

Hmm... I think in that case, my comments are irrelevant!

Thanks for making the PR 👍

Also, just to be clear: The title should rather say,

"Exporting component types from @types/theme-ui__components."

Sorry I didn’t express myself clearly. I assumed "types" is implicit in the context of DefinitelyTyped. A bit lazy of me.

prateekkathal · 2020-01-29T16:17:26Z

types/theme-ui/index.d.ts

@@ -157,14 +154,6 @@ export interface SxProps {

 type SxComponent<T extends SxProps = IntrinsicSxElements['div']> = React.ComponentClass<T & { as?: React.ElementType }>;

-export const Box: SxComponent;


Again, the goal is to define component props, not make them exportable from the main package itself.

prateekkathal · 2020-01-29T16:19:37Z

@hasparus It seems like there is a PR sitting for styled-system #41903 & issue still unresolved for theme-ui #40897

I'd defer merging this PR as the proposed changes might fix older versions of theme-ui types. If a breaking change must be introduced, it should be made ensuring previous builds don't have errors.

typescript-bot · 2020-01-29T16:24:37Z

@hasparus 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 or comments. Thank you!

prateekkathal · 2020-01-30T17:00:44Z

@hasparus I am gonna wait on the updated version of Styled System types to be merged and will then approve this one.

Thanks again for the PR! 👍

typescript-bot · 2020-01-30T17:24:46Z

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

hasparus added 2 commits January 28, 2020 17:21

Reexport @theme-ui/components from theme-ui

6715100

Rename initialColorMode to initialColorModeName

13a79e1

hasparus mentioned this pull request Jan 28, 2020

Typescript support system-ui/theme-ui#121

Closed

typescript-bot added the The Travis CI build failed label Jan 28, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jan 28, 2020

Change required TS version of @types/theme-ui to 3.5

aa0d7b1

typescript-bot added Awaiting reviewer feedback and removed The Travis CI build failed labels Jan 28, 2020

prateekkathal suggested changes Jan 29, 2020

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Jan 29, 2020

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express labels Jan 30, 2020

prateekkathal approved these changes Jan 30, 2020

View reviewed changes

weswigham merged commit c388d9e into DefinitelyTyped:master Jan 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reexport @theme-ui/components from theme-ui #41915

Reexport @theme-ui/components from theme-ui #41915

hasparus commented Jan 28, 2020

typescript-bot commented Jan 28, 2020 •

edited

Loading

typescript-bot commented Jan 28, 2020 •

edited

Loading

typescript-bot commented Jan 28, 2020

prateekkathal commented Jan 28, 2020

hasparus commented Jan 28, 2020 •

edited

Loading

hasparus commented Jan 28, 2020

prateekkathal Jan 29, 2020

hasparus Jan 30, 2020

prateekkathal Jan 30, 2020 •

edited

Loading

hasparus Jan 30, 2020

prateekkathal Jan 29, 2020

prateekkathal commented Jan 29, 2020

typescript-bot commented Jan 29, 2020

prateekkathal commented Jan 30, 2020

typescript-bot commented Jan 30, 2020

		import { Interpolation, SerializedStyles } from '@emotion/serialize';
		export * from '@theme-ui/components';

		@@ -157,14 +154,6 @@ export interface SxProps {

		type SxComponent<T extends SxProps = IntrinsicSxElements['div']> = React.ComponentClass<T & { as?: React.ElementType }>;

		export const Box: SxComponent;

Reexport @theme-ui/components from theme-ui #41915

Reexport @theme-ui/components from theme-ui #41915

Conversation

hasparus commented Jan 28, 2020

typescript-bot commented Jan 28, 2020 • edited Loading

typescript-bot commented Jan 28, 2020 • edited Loading

typescript-bot commented Jan 28, 2020

theme-ui/v*

theme-ui__components/v*

prateekkathal commented Jan 28, 2020

hasparus commented Jan 28, 2020 • edited Loading

hasparus commented Jan 28, 2020

prateekkathal Jan 29, 2020

Choose a reason for hiding this comment

hasparus Jan 30, 2020

Choose a reason for hiding this comment

prateekkathal Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

hasparus Jan 30, 2020

Choose a reason for hiding this comment

prateekkathal Jan 29, 2020

Choose a reason for hiding this comment

prateekkathal commented Jan 29, 2020

typescript-bot commented Jan 29, 2020

prateekkathal commented Jan 30, 2020

typescript-bot commented Jan 30, 2020

typescript-bot commented Jan 28, 2020 •

edited

Loading

typescript-bot commented Jan 28, 2020 •

edited

Loading

hasparus commented Jan 28, 2020 •

edited

Loading

prateekkathal Jan 30, 2020 •

edited

Loading