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

Refine style types #1554

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Refine style types #1554

wants to merge 6 commits into from

Conversation

kof
Copy link
Member

@kof kof commented Sep 13, 2021

Trying to make the types more accurate and ideally improve error messages and tsc performance in the end.

@kof
Copy link
Member Author

kof commented Sep 13, 2021

This is a work in progress, before we can merge it I would like to turn all tests into ts files so that we have everything covered by the existing tests.

But I would love a pre-review already at this stage, because this is going to be a big PR.

@kof kof requested a review from a team September 13, 2021 15:05
})
}
})

// Nested declarations are banned (double nest test)
const noThemeArg6 = createUseStyles<string, MyProps, MyTheme>({
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't clearly understand what this is testing, because nesting is not generally disallowed, its just only working with nested syntax with &

}
})

// Nested declarations are banned (triple nest test)
Copy link
Member Author

Choose a reason for hiding this comment

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

why would one need double nest and triple nest test, is there code that is explicitly working in one of them and not in another?

Copy link
Member

Choose a reason for hiding this comment

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

I know merging of some commits happened earlier. Not sure if this PR holds any of those.

My comments on those kinds of tests are probably misleading. What it should say is probably something like

Nested function declarations are banned

In terms of function declarations, no, I'm not aware of any existing code that tries to nest function declarations. But I thought that I read on the docs somewhere (or in a console warning) that nested functions were not supported. This test was just to ensure that the types would also error out if someone tried to create a nested function (before the runtime bites them).

I did "double" and "triple" failure tests for function declarations just to make sure the type was appropriately defined. This was for two main reasons:

  1. If a person was doing regular nesting that didn't involve functions, they should be able to define a stand-alone, un-nested function wherever they please.
  2. If a person tried to be sneaky and say, "Maybe I can nest function declarations if I start at a deeper point in the nest", this test ensures that they'd be prevented from doing so.

Theoretically speaking, a person could nest as much as they want. But testing every Nth scenario would be impractical. I stopped at 2nd and 3rd level nesting to verify that our recursive types were properly defined.

Copy link
Member

Choose a reason for hiding this comment

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

This probably also answers #1554 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

The confusing part was triple test, if we just forbid double nest its already solves the problem

Copy link
Member

Choose a reason for hiding this comment

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

Okay. If you think the double nest is sufficient, I think we can remove all the triple-nest tests. Makes the file shorter 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean if double nesting is already prevented, x nesting is also prevented. It was just confusing to see 3x level of nesting because I start thinking why would you need to test a 3rd level, whats special about it

export type Styles<
Name extends string | number | symbol = string,
Props = unknown,
Data = unknown,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was supposed to be Data all along, the fact it can be Props is up to user, it could be anything

Base automatically changed from upgrade-ts-4.4.2 to master September 18, 2021 10:10
@ITenthusiasm
Copy link
Member

I haven't looked at the [currently last] commit 04e94fb yet, but the preceding commits seem fine.

Regarding 04e94fb, it might be helpful to split out the prettier changes into a preceding (or succeeding) chore commit just to make the reviewing process a little easier. (A dev could speed through the prettier changes and then give more special attention to the type-related changes that are found in a separate commit.)

| Func<Data, Theme, JssStyle<undefined, undefined> | string | null | undefined>
| MinimalObservable<JssStyle | string | null | undefined>
>
| Record<`@keyframes ${string}`, Record<'from' | 'to' | `${number}%`, JssStyle<Data, Theme>>>
Copy link
Member

Choose a reason for hiding this comment

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

Is need to add @global here?

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