-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
### Goal Make it easier for frameworks to wrap our TypeScript code. Refs #4225 ### How **Isolate framework-specific code from framework-agnostic code.** Specifically, this PR: * Moves all component definitions from `index.ts` to `component.ts` - `index.ts` retains a default component export for backward compatibility * Moves all component-dependent types from `types.ts` to `component.ts` - Every framework-related type is now quarantined in a single file: `component.ts` - All other files are now purely framework-agnostic, and can be safely wrapped by frameworks that don't use our components * Makes `import` paths more specific by switching from `@material/foo/index` to e.g. `@material/foo/foundation` or `@material/foo/types` - The only exception is `component.ts` files: Since they're basically our default "framework", they can safely import _other_ "framework" types via `@material/foo/index` * Updates class & interface export syntax to isolate the `default` export line for future removal if necessary: ```ts export class MDCFooFoundation { // ... } export default MDCFooFoundation; ``` ### Packages * [x] `animation` * [x] `checkbox` * [x] `chips` * [ ] `dialog` * [x] `dom` * [ ] `drawer` * [ ] `floating-label` * [ ] `form-field` * [ ] `grid-list` * [ ] `icon-button` * [ ] `icon-toggle` _(deprecated)_ * [ ] `line-ripple` * [ ] `linear-progress` * [ ] `list` * [ ] `menu` * [ ] `menu-surface` * [ ] `notched-outline` * [ ] `radio` * [x] `ripple` * [ ] `select` * [x] `selection-control` * [ ] `slider` * [ ] `snackbar` * [ ] `switch` * [ ] `tab` * [ ] `tab-bar` * [ ] `tab-indicator` * [ ] `tab-scroller` * [ ] `tab-tabs` _(deprecated)_ * [ ] `textfield` * [ ] `toolbar` _(deprecated)_ * [ ] `top-app-bar`
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4407 +/- ##
===================================================
+ Coverage 99.05% 99.06% +<.01%
===================================================
Files 98 130 +32
Lines 6166 6230 +64
Branches 808 811 +3
===================================================
+ Hits 6108 6172 +64
Misses 57 57
Partials 1 1
Continue to review full report at Codecov.
|
All 624 screenshot tests passed for commit d4320a3 vs. |
All 624 screenshot tests passed for commit 80223a1 vs. |
All 624 screenshot tests passed for commit 7e85e42 vs. |
packages/mdc-dom/externs.d.ts
Outdated
@@ -24,3 +24,7 @@ | |||
declare interface Element { | |||
msMatchesSelector?: (selector: string) => boolean; | |||
} | |||
|
|||
declare interface Window { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does moving this from typings
require more things to rely on dom
? Are we sure we have the dependency everywhere it's needed? Does this now need to be included explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. In our repo, TypeScript doesn't seem to care where this file is. Some wrapper frameworks might run into problems though, and would need to add a dep on mdc-dom
. Would you rather put this in mdc-base
? (I actually considered doing that, but mdc-dom
seemed more semantically correct. But maybe convenience is more important than semantics.)
@moog16 @mmalerba Is it OK that these types have the same names as existing types in lib.dom.d.ts
? Does TypeScript somehow merge the two type definitions together or something? Or should we create a new interface with a different name that extends Window
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this is actually to extend what is in lib.dom.d.ts
. The TS lib.dom.d.ts
Window
definition does not have a CSS
property, which is why we can reference it in our code.
I think this actually needs to be in a package and exported. Our clients/users using our packages, might need this for their code as well.
If we don't want to rely on dom
as much, I suggest moving it to base
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, TS merges interfaces: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Moved from mdc-dom
to mdc-base
.
All 624 screenshot tests passed for commit 5ef72ad vs. |
All 624 screenshot tests passed for commit 1b8e912 vs. |
All 624 screenshot tests passed for commit 1e68882 vs. |
…Surface`, which would break backward compatibility)
All 624 screenshot tests passed for commit 03db3ea vs. |
All 624 screenshot tests passed for commit 9406e0c vs. |
All 624 screenshot tests passed for commit 07bc5ea vs. |
}; | ||
export {util}; // New namespace | ||
export * from './types'; | ||
export * from './util'; // Old namespace for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In master, animation also exports transformStyleProperties
, which seems to have disappeared from the package on the TS branch?
const transformStyleProperties = ['transform', 'WebkitTransform', 'MozTransform', 'OTransform', 'MSTransform'];
It seems very unlikely that anything external would rely on this though... (We relied on it in linear-progress but it no longer does; it uses getCorrectPropertyName
now instead.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I guess that's technically a breaking change...
Should I mirror this with a PR in master to ensure it ends up in the changelog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible and reasonable to do that in master? (I guess it comes down to being able to make a similar change to linear-progress in master.)
All 624 screenshot tests passed for commit eb36503 vs. |
All 624 screenshot tests passed for commit a09f608 vs. |
All 624 screenshot tests passed for commit 56a0650 vs. |
I feel that ken and andy have been focused on this PR that it is not necessary for me to continue.
Mirrors #4407 (comment) from the `feat/typescript` branch. BREAKING CHANGE: The `transformStyleProperties` array export has been removed from `mdc-animation`. Please use `getCorrectPropertyName(window, 'transform')` instead.
Goal
Make it easier for frameworks to wrap our TypeScript code.
Refs #4225
How
Isolate framework-specific code from framework-agnostic code.
Specifically, this PR:
index.ts
tocomponent.ts
index.ts
is now purely re-exporting other filestypes.ts
tocomponent.ts
component.ts
import
paths more specific by switching from@material/foo/index
to e.g.@material/foo/foundation
or@material/foo/types
component.ts
files: Since they're basically our default "framework", they can safely import other "framework" types via@material/foo/index
default
export line for future removal if necessary:typings/custom.d.ts
andtypings/dom.ie.d.ts
intopackages/mdc-dom/externs.d.ts
so the types will be publicly visibleMDCFooFactory
types for components that need them (e.g.,MDCRipple
,MDCList
)Packages
animation
checkbox
chips
dialog
dom
drawer
floating-label
form-field
grid-list
icon-button
icon-toggle
(deprecated)line-ripple
linear-progress
list
menu
menu-surface
notched-outline
radio
ripple
select
selection-control
slider
snackbar
switch
tab
tab-bar
tab-indicator
tab-scroller
tab-tabs
(deprecated)textfield
toolbar
(deprecated)top-app-bar