-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
(WIP) Upgrade to TypeScript 4.1 #16449
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,7 +110,7 @@ | |
"postcss-modules": "2.0.0", | ||
"ts-loader": "8.0.14", | ||
"ts-jest": "24.0.1", | ||
"typescript": "3.7.2", | ||
"typescript": "4.1.5", | ||
"webpack-dev-server": "4.0.0-beta.0", | ||
"webpack-cli": "4.3.1", | ||
"webpack": "5.21.2" | ||
|
@@ -125,9 +125,11 @@ | |
], | ||
"nohoist": [ | ||
"packages/web-components/webpack", | ||
"packages/web-components/@microsoft/eslint-config-fast-dna", | ||
"packages/web-components/@storybook/**", | ||
"packages/web-components/typescript", | ||
ecraig12345 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"packages/web-components/typescript/**", | ||
"packages/web-components/**/typescript", | ||
Comment on lines
130
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This very thoroughly prevents web-components' different versions of typescript from being hoisted.
|
||
"packages/web-components/ts-loader", | ||
"packages/web-components/ts-loader/**", | ||
"packages/web-components/ts-node", | ||
|
@@ -154,19 +156,12 @@ | |
"scripts/package.json" | ||
], | ||
"versionGroups": [ | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not directly related but this is no longer needed; as of some previous PR test-bundles now has the same webpack version as everything else |
||
"packages": [ | ||
"test-bundles" | ||
], | ||
"dependencies": [ | ||
"webpack" | ||
] | ||
}, | ||
{ | ||
"packages": [ | ||
"@fluentui/web-components" | ||
], | ||
"dependencies": [ | ||
"@microsoft/api-extractor", | ||
"mocha", | ||
"ts-loader", | ||
"ts-node", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,8 +31,8 @@ | |
"@fluentui/style-utilities": "^8.0.2", | ||
"@fluentui/theme": "^2.0.2", | ||
"@fluentui/utilities": "^8.0.2", | ||
"@microsoft/api-extractor-model": "7.7.1", | ||
"@microsoft/tsdoc": "0.12.14", | ||
"@microsoft/api-extractor-model": "7.12.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must be upgraded due to dep on typescript |
||
"@microsoft/tsdoc": "0.12.24", | ||
"fs-extra": "^8.1.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,7 +4,7 @@ import * as React from 'react'; | |||||
|
||||||
import * as consoleUtil from '../consoleUtil'; | ||||||
|
||||||
const TestComponent: React.FC<{ trigger?: React.ReactElement | null }> = props => { | ||||||
const TestComponent: React.FC<{ trigger?: React.ReactElement }> = props => { | ||||||
return useTriggerElement(props); | ||||||
}; | ||||||
|
||||||
|
@@ -24,7 +24,7 @@ describe('useTriggerElement', () => { | |||||
}); | ||||||
|
||||||
it('"trigger" can be null', () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 will update this shortly--I'd considered this change before but I wasn't sure if null was special and specifically needed to be supported for some reason. |
||||||
const wrapper = mount(<TestComponent trigger={null} />); | ||||||
const wrapper = mount(<TestComponent trigger={null as any} />); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
expect(wrapper.children()).toHaveLength(0); | ||||||
}); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,7 +71,7 @@ export class RefFindNode extends React.Component<RefProps> { | |
componentWillUnmount() { | ||
handleRef(this.props.innerRef, null); | ||
|
||
delete this.prevNode; | ||
this.prevNode = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @miroslavstastny this part was a fix of memory leak. Is this change safe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @ecraig12345 - memory-leak wise, the two should be equal. |
||
} | ||
|
||
render() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ export class RefForward extends React.Component<RefProps> { | |
} | ||
|
||
componentWillUnmount() { | ||
delete this.currentNode; | ||
this.currentNode = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @miroslavstastny this part was a fix of memory leak. Is this change safe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless there's some nuance that I'm not aware of, I thought that removing the reference by either deleting or setting to another value should work to address memory leaks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @ecraig12345 - memory-leak wise, the two should be equal. |
||
} | ||
|
||
render() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,7 @@ export type ComponentWithAs<TElementType extends keyof JSX.IntrinsicElements = ' | |
|
||
/** | ||
* A hack to simplify the resolution for ComponentWithAs. | ||
* @see https://github.com/microsoft/fluentui/pull/13841 | ||
* See https://github.com/microsoft/fluentui/pull/13841 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might have been needed due to the new API Extractor version complaining about unrecognized tags |
||
*/ | ||
readonly __PRIVATE_PROPS?: Omit<PropsOfElement<TElementType>, 'as' | keyof TProps> & { as?: TElementType } & TProps; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,9 @@ | ||
import * as React from 'react'; | ||
import { CalendarDayBase } from './CalendarDay.base'; | ||
import { styles } from './CalendarDay.styles'; | ||
import { styled } from '../../../Utilities'; | ||
import { ICalendarDayProps } from './CalendarDay.types'; | ||
|
||
export const CalendarDay = styled(CalendarDayBase, styles, undefined, { scope: 'CalendarDay' }); | ||
export const CalendarDay: React.FunctionComponent<ICalendarDayProps> = styled(CalendarDayBase, styles, undefined, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is required to prevent inferred imports in the .d.ts, which API Extractor now complains about. I think I'm going to separate out this and the other date-time stuff into another PR, because it also included some general types cleanup to more accurately reflect actual usage. |
||
scope: 'CalendarDay', | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,12 @@ | ||
import * as React from 'react'; | ||
import { CalendarMonthBase } from './CalendarMonth.base'; | ||
import { getStyles } from './CalendarMonth.styles'; | ||
import { styled } from '../../../Utilities'; | ||
import { ICalendarMonthProps } from './CalendarMonth.types'; | ||
|
||
export const CalendarMonth = styled(CalendarMonthBase, getStyles, undefined, { scope: 'CalendarMonth' }); | ||
export const CalendarMonth: React.FunctionComponent<ICalendarMonthProps> = styled( | ||
CalendarMonthBase, | ||
getStyles, | ||
undefined, | ||
{ scope: 'CalendarMonth' }, | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,12 @@ | ||
import * as React from 'react'; | ||
import { getStyles } from './CalendarYear.styles'; | ||
import { styled } from '../../../Utilities'; | ||
import { CalendarYearBase } from './CalendarYear.base'; | ||
import { ICalendarYearProps } from './CalendarYear.types'; | ||
|
||
export const CalendarYear = styled(CalendarYearBase, getStyles, undefined, { scope: 'CalendarYear' }); | ||
export const CalendarYear: React.FunctionComponent<ICalendarYearProps> = styled( | ||
CalendarYearBase, | ||
getStyles, | ||
undefined, | ||
{ scope: 'CalendarYear' }, | ||
); |
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.
eslint-config-fast-dna also has a dep on typescript, so hopefully not hoisting it will help prevent issues.
ts-loader is tentatively using the same version now and doesn't need nohoist config.