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

[TypeScript] Replace ThemeType Absolute Imports with Relative Imports #510

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

michaeljaltamirano
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano commented Nov 9, 2020

We use absolute imports for convenience for much of our CONSTANTS usage. As a result, rollup helps us turn something like import { SPACER } from 'src/constants' into something like import SPACER from '../../../constants/spacer/index.js';.

TypeScript compilation does not do this by design:

  1. "TypeScript doesn't modify import paths as part of compilation - you should always write the path you want to appear in the emitted JS"
  2. "Our general take on this is that you should write the import path that works at runtime"

As a result, consumer apps have no way to understand our ThemeType in declaration files. The below is an example of radiance-ui/lib/utils/injectGlobalStyles/style.d.ts

import 'focus-visible';
import { ThemeType } from 'src/constants/themes/types';
export declare const resetStyles: string;
export declare const brandStyles: (theme: ThemeType) => string;
//# sourceMappingURL=style.d.ts.map

ThemeType ends up casting to any because the path is not resolvable.

This PR updates all of our ThemeType imports (and some others) to pull from src/constants relatively. As a result the tsc output now points to the actual file definition:

import 'focus-visible';
import { ThemeType } from '../../constants';
export declare const resetStyles: string;
export declare const brandStyles: (theme: ThemeType) => string;
//# sourceMappingURL=style.d.ts.map

There were two alternatives considered, both of which seemed sub-optimal since (I believe) our preference should be for things to "work out of the box":

  1. We could use path mapping in tsconfig.json to resolve the absolute path in the consumer app, like so:
"paths": {
  "emotion-theming": ["jsx/types/modules/emotion-theming"],
  "@emotion/styled": ["jsx/types/modules/@emotion/styled"],
  "src/constants/themes/types": ["../../node_modules/radiance-ui/lib/constants/themes/types"]
},

However, it seems inadequate to add an additional usage step in every consumer app, rather than address the issue at the source.

  1. We could use some open source tools to modify path aliases: ttypescript (not a typo), ts-transform-paths, etc.

However, it seems likely that making our configuration more complex with two additional tools, some of which do not appear to have more recent updates, would likely make it harder for us to stay up-to-date with TypeScript in the future, and cause other unexpected problems.

When considered along-with the recommendation by the TypeScript team to write the code as it will exist at runtime, the most appropriate course for a component library would seem to be to more heavily rely on relative imports. This PR does not yet include documentation/app-configuration to enforce this, but it unblocks our efforts to release a v13. Looking at component library best practices and updating accordingly could be a cooldown task during the holidays.

@snags88 snags88 temporarily deployed to curology-radiance-pr-510 November 9, 2020 18:00 Inactive
@@ -1,3 +1,3 @@
export { primaryTheme } from './primaryTheme';
export { secondaryTheme } from './secondaryTheme';
export { COLORS_PROP_TYPES } from './types';
export * from './types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than have in all our source files:

import { SPACER } from '../../constants';
import { ThemeType } from '../../constants/theme/types';

We can just combine them for convenience:

import { SPACER, ThemeType } from '../../constants';

Copy link
Contributor

@daigof daigof left a comment

Choose a reason for hiding this comment

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

Looks good, I was trying to figure it out this TS eslint

image

have you seen that? Do you think we can do something in emotion-themeing file? (not related to this PR (I think))

@michaeljaltamirano
Copy link
Contributor Author

Looks good, I was trying to figure it out this TS eslint

image

have you seen that? Do you think we can do something in emotion-themeing file? (not related to this PR (I think))

Let me take a look. I added some stricter rules but I've also added some disables for those rules when they're in test/etc. files that are less important than functional code.

@michaeljaltamirano
Copy link
Contributor Author

michaeljaltamirano commented Nov 9, 2020

Looks good, I was trying to figure it out this TS eslint
image
have you seen that? Do you think we can do something in emotion-themeing file? (not related to this PR (I think))

Let me take a look. I added some stricter rules but I've also added some disables for those rules when they're in test/etc. files that are less important than functional code.

@daigof I pulled in this repo with fresh node_modules and didn't see this error (ran yarn run eslint on the file). I'm going to merge it in so I can do a beta release--could you double-check it's not a stale editor/modules issue?

@michaeljaltamirano michaeljaltamirano merged commit 89b240e into master Nov 9, 2020
@michaeljaltamirano michaeljaltamirano deleted the typescript/relative-imports branch November 9, 2020 18:57
daigof added a commit that referenced this pull request Nov 9, 2020
* [Theme setup] Initial setup and Accordion example (#436)

* initial setup

* convert Acoordion,add  theme suport to stories

* complete accordion example

* shouldShowForEnvironment refactor

* commit snapshot size

* v13.0.0-alpha.1

* Replace swap button with addon-toolbars to fix visual regression

Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com>

* [Theming] Add built-in TypeScript support (#463)

* initial setup

* convert Acoordion,add  theme suport to stories

* complete accordion example

* shouldShowForEnvironment refactor

* commit snapshot size

* v13.0.0-alpha.1

* Commit example with Rollup + TypeScript resolution

* Commit hard-coded import

* Revert "Commit hard-coded import"

This reverts commit 9713df2.

* PR Cleanup

* Remove rollup plugin

* Move type resolution files out of utils and into types folder

* Commit master size-snapshot

* Push last size-snapshot

Co-authored-by: Diego Fortes <daigof@gmail.com>

* [Theming] Convert Component Usage (Part I) (#467)

* Convert for Accordion.Thumbnails

* Convert easy cases: styled component and normal component usage

* Add @types/enzyme-adapter-react-16

* Pass in blank string instead of undefined to maintain some semblance of distinction (for now)

* Convert setupTests.js to ts

* Get tests passing, check on chromatic

* [Theming] Convert Component Usage (Part II, Buttons) (#475)

* Bump @types/react from 16.9.53 to 16.9.55 (#473)

* Bump @types/react-dom from 16.9.8 to 16.9.9 (#469)

* Bump @types/enzyme from 3.10.7 to 3.10.8 (#470)

* Bump lint-staged from 10.4.0 to 10.5.0 (#468)

* Rename primary and secondary files to match default export name, export new COLORS_PROP_TYPES for aliased values

* Export values as const for type inference, add secondary tints for lavender usage in buttons

* Button update: add theming, constrict typing, remove unnecessary css imports (when in-lining in styled component)

* Update size snapshot, new items due to new COLORS_PROP_TYPES usage

* Decorate tests

* Add back base button css composition, since typography depends on it

* Tighten textColor typing, too

* Override prettier, TODO: use prettier-eslint

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* [Theming] Convert Components (Part III) (#476)

* Update global styles and storybook preview configuration

* Update Callout, Chip, LoadingSpinner, and ProgressBar

* Update DialogModal, Tabs, and Toggle

* Update Dropdown

* Update SelectorButton styles, modify eslint rule

* Convert COntainer, ImmersiveModal, OptionButton

* Last commits: Container and Field, and update .size-snapshot

* Rename prop to get off DOM

* [Theming] Convert Components (Part IV) (#477)

* Bump @types/react from 16.9.53 to 16.9.55 (#473)

* Bump @types/react-dom from 16.9.8 to 16.9.9 (#469)

* Bump @types/enzyme from 3.10.7 to 3.10.8 (#470)

* Bump lint-staged from 10.4.0 to 10.5.0 (#468)

* Rename primary and secondary files to match default export name, export new COLORS_PROP_TYPES for aliased values

* Export values as const for type inference, add secondary tints for lavender usage in buttons

* Button update: add theming, constrict typing, remove unnecessary css imports (when in-lining in styled component)

* Update size snapshot, new items due to new COLORS_PROP_TYPES usage

* Decorate tests

* Add back base button css composition, since typography depends on it

* Tighten textColor typing, too

* Override prettier, TODO: use prettier-eslint

* Commit Typography WIP, save for last

* Fix build, update .size-snapshot, replace all TYPOGRAPHY_STYLE usages, including story

* Reconfigure stories/tsconfig, export ThemeColors instead for improved DX

* Fix bizarre formatting

* Fix formatting pt. 2

* One last eslint fix and prettier write cleanup

* try running through eslint-plugin-prettier

* Conform to docs order

* Remove floating semicolons

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* [Theming] Update Colors Export (#480)

* Bump @types/react from 16.9.53 to 16.9.55 (#473)

* Bump @types/react-dom from 16.9.8 to 16.9.9 (#469)

* Bump @types/enzyme from 3.10.7 to 3.10.8 (#470)

* Bump lint-staged from 10.4.0 to 10.5.0 (#468)

* Bump typescript from 4.0.3 to 4.0.5 (#471)

* Bump chromatic from 5.2.0 to 5.3.0 (#478)

* Bump @react-aria/focus from 3.2.2 to 3.2.3 (#479)

* Mass commit: add TODO to existing COLORS export, fix lingering COLORS imports, update stories

* Add react-is as devDependency to fix install warning

* Fix visual regression in Available Constants story

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* v13.0.0-alpha.2

* [Theming] Fix tsc output (#498)

* Re-arrange new files for theming to fix lib type file structure

* Fix window.matchMedia type by fleshing it out

* v13.0.0-alpha.3

* [Theming] Update Test Suite (#505)

* [Theming] Update Declarations  (#504)

* Merge declaration changes from master

* Rename code comment

* Do not make Button onClick event potentially undefined

* Keep Dropdown value type consistent

* v13.0.0-alpha.4

* Push size changes, too

* Narrow ReactNode to JSX.Element in ImmersiveModal title

* Simplify generics usage

* refactor fonts to theme

* add test

* refactor out test util function

* reafctor util tester

* [TypeScript] Replace ThemeType Absolute Imports with Relative Imports (#510)

* Move ThemeType (and associated imports) to relative imports instead of absolute

* Update .size-snapshot.json

Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
michaeljaltamirano added a commit that referenced this pull request Nov 10, 2020
* [Themes] - Add fonts to themes (#509)

* [Theme setup] Initial setup and Accordion example (#436)

* initial setup

* convert Acoordion,add  theme suport to stories

* complete accordion example

* shouldShowForEnvironment refactor

* commit snapshot size

* v13.0.0-alpha.1

* Replace swap button with addon-toolbars to fix visual regression

Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com>

* [Theming] Add built-in TypeScript support (#463)

* initial setup

* convert Acoordion,add  theme suport to stories

* complete accordion example

* shouldShowForEnvironment refactor

* commit snapshot size

* v13.0.0-alpha.1

* Commit example with Rollup + TypeScript resolution

* Commit hard-coded import

* Revert "Commit hard-coded import"

This reverts commit 9713df2.

* PR Cleanup

* Remove rollup plugin

* Move type resolution files out of utils and into types folder

* Commit master size-snapshot

* Push last size-snapshot

Co-authored-by: Diego Fortes <daigof@gmail.com>

* [Theming] Convert Component Usage (Part I) (#467)

* Convert for Accordion.Thumbnails

* Convert easy cases: styled component and normal component usage

* Add @types/enzyme-adapter-react-16

* Pass in blank string instead of undefined to maintain some semblance of distinction (for now)

* Convert setupTests.js to ts

* Get tests passing, check on chromatic

* [Theming] Convert Component Usage (Part II, Buttons) (#475)

* Bump @types/react from 16.9.53 to 16.9.55 (#473)

* Bump @types/react-dom from 16.9.8 to 16.9.9 (#469)

* Bump @types/enzyme from 3.10.7 to 3.10.8 (#470)

* Bump lint-staged from 10.4.0 to 10.5.0 (#468)

* Rename primary and secondary files to match default export name, export new COLORS_PROP_TYPES for aliased values

* Export values as const for type inference, add secondary tints for lavender usage in buttons

* Button update: add theming, constrict typing, remove unnecessary css imports (when in-lining in styled component)

* Update size snapshot, new items due to new COLORS_PROP_TYPES usage

* Decorate tests

* Add back base button css composition, since typography depends on it

* Tighten textColor typing, too

* Override prettier, TODO: use prettier-eslint

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* [Theming] Convert Components (Part III) (#476)

* Update global styles and storybook preview configuration

* Update Callout, Chip, LoadingSpinner, and ProgressBar

* Update DialogModal, Tabs, and Toggle

* Update Dropdown

* Update SelectorButton styles, modify eslint rule

* Convert COntainer, ImmersiveModal, OptionButton

* Last commits: Container and Field, and update .size-snapshot

* Rename prop to get off DOM

* [Theming] Convert Components (Part IV) (#477)

* Bump @types/react from 16.9.53 to 16.9.55 (#473)

* Bump @types/react-dom from 16.9.8 to 16.9.9 (#469)

* Bump @types/enzyme from 3.10.7 to 3.10.8 (#470)

* Bump lint-staged from 10.4.0 to 10.5.0 (#468)

* Rename primary and secondary files to match default export name, export new COLORS_PROP_TYPES for aliased values

* Export values as const for type inference, add secondary tints for lavender usage in buttons

* Button update: add theming, constrict typing, remove unnecessary css imports (when in-lining in styled component)

* Update size snapshot, new items due to new COLORS_PROP_TYPES usage

* Decorate tests

* Add back base button css composition, since typography depends on it

* Tighten textColor typing, too

* Override prettier, TODO: use prettier-eslint

* Commit Typography WIP, save for last

* Fix build, update .size-snapshot, replace all TYPOGRAPHY_STYLE usages, including story

* Reconfigure stories/tsconfig, export ThemeColors instead for improved DX

* Fix bizarre formatting

* Fix formatting pt. 2

* One last eslint fix and prettier write cleanup

* try running through eslint-plugin-prettier

* Conform to docs order

* Remove floating semicolons

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* [Theming] Update Colors Export (#480)

* Bump @types/react from 16.9.53 to 16.9.55 (#473)

* Bump @types/react-dom from 16.9.8 to 16.9.9 (#469)

* Bump @types/enzyme from 3.10.7 to 3.10.8 (#470)

* Bump lint-staged from 10.4.0 to 10.5.0 (#468)

* Bump typescript from 4.0.3 to 4.0.5 (#471)

* Bump chromatic from 5.2.0 to 5.3.0 (#478)

* Bump @react-aria/focus from 3.2.2 to 3.2.3 (#479)

* Mass commit: add TODO to existing COLORS export, fix lingering COLORS imports, update stories

* Add react-is as devDependency to fix install warning

* Fix visual regression in Available Constants story

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* v13.0.0-alpha.2

* [Theming] Fix tsc output (#498)

* Re-arrange new files for theming to fix lib type file structure

* Fix window.matchMedia type by fleshing it out

* v13.0.0-alpha.3

* [Theming] Update Test Suite (#505)

* [Theming] Update Declarations  (#504)

* Merge declaration changes from master

* Rename code comment

* Do not make Button onClick event potentially undefined

* Keep Dropdown value type consistent

* v13.0.0-alpha.4

* Push size changes, too

* Narrow ReactNode to JSX.Element in ImmersiveModal title

* Simplify generics usage

* refactor fonts to theme

* add test

* refactor out test util function

* reafctor util tester

* [TypeScript] Replace ThemeType Absolute Imports with Relative Imports (#510)

* Move ThemeType (and associated imports) to relative imports instead of absolute

* Update .size-snapshot.json

Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* [Colors] Add 'transparent' color (#512)

* Add transparent as valid COLORS alias/value

Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
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