(WIP) Upgrade to TypeScript 4.1 #16449
Conversation
ecraig12345
requested review from
dzearing,
Hotell,
ling1726,
andrefcdias,
joschect and
kubkon
apps/vr-tests/package.json
Outdated
| @@ -35,7 +35,7 @@ | |||
| "@types/react-dom": "16.8.4", | |||
| "@types/react": "16.8.25", | |||
| "@types/webpack-env": "1.15.1", | |||
| "awesome-typescript-loader": "^3.2.3", | |||
| "awesome-typescript-loader": "^5.2.1", | |||
There was a problem hiding this comment.
TBD whether this upgrade is necessary/desirable and whether it necessitates other changes
package.json
Outdated
| "packages/web-components/typescript/**", | ||
| "packages/web-components/ts-loader", | ||
| "packages/web-components/ts-loader/**", | ||
| "packages/web-components/@microsoft/api-extractor", |
There was a problem hiding this comment.
If we leave web-components on a different TS version, it also needs its own API Extractor version (which shouldn't be hoisted to avoid interference from API Extractor's additional separate TS version)
| "packages/web-components/ts-loader/**", | ||
| "packages/web-components/@microsoft/api-extractor", | ||
| "packages/web-components/@microsoft/api-extractor/**", | ||
| "packages/web-components/@microsoft/eslint-config-fast-dna", |
There was a problem hiding this comment.
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.
package.json
Outdated
| @@ -104,7 +104,8 @@ | |||
| "resolutions": { | |||
| "eslint": "^7.1.0", | |||
| "autoprefixer": "9.7.6", | |||
| "copy-to-clipboard": "3.2.0" | |||
| "copy-to-clipboard": "3.2.0", | |||
| "**/@fluentui/scripts/**/typescript": "4.1.3" | |||
There was a problem hiding this comment.
The reason for this line is to force API Extractor to use TS 4.1.3 rather than 4.0.5. After MUCH fighting with yarn resolutions I finally figured out that you need the leading ** to prevent hoisting from something under a monorepo package.
Resolutions docs: https://classic.yarnpkg.com/en/docs/selective-version-resolutions/
Full spec: https://github.com/yarnpkg/rfcs/blob/master/implemented/0000-selective-versions-resolutions.md
| @@ -9,7 +9,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "ability-attributes-generator": "^0.0.8", | |||
| "typescript": "3.7.2" | |||
| "typescript": "4.1.3" | |||
There was a problem hiding this comment.
It's possible that with the improvements I made to the root nohoist config, this won't be necessary now. (The other possibility would be adding a nohoist line for packages/web-components/**/typescript.) But might be better to do that separately. ref #14371
| @@ -63,7 +63,7 @@ export const createContext = <Value>(defaultValue: Value, options: CreateContext | |||
| context.Provider = createProvider<Value>(context.Provider) as any; | |||
|
|
|||
| // We don't support Consumer API | |||
| delete context.Consumer; | |||
| delete (context as any).Consumer; | |||
There was a problem hiding this comment.
delete of required members is no longer allowed
Asset size changes
Baseline commit: eca0330d791682031d691cdd8fe6fb0a65c159ab (build) |
| @@ -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.
@miroslavstastny this part was a fix of memory leak. Is this change safe?
There was a problem hiding this comment.
Agree with @ecraig12345 - memory-leak wise, the two should be equal.
| @@ -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.
@miroslavstastny this part was a fix of memory leak. Is this change safe?
There was a problem hiding this comment.
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.
Agree with @ecraig12345 - memory-leak wise, the two should be equal.
| @@ -24,7 +24,7 @@ describe('useTriggerElement', () => { | |||
| }); | |||
|
|
|||
| it('"trigger" can be null', () => { | |||
| const wrapper = mount(<TestComponent trigger={null} />); | |||
| const wrapper = mount(<TestComponent trigger={null as any} />); | |||
There was a problem hiding this comment.
| const wrapper = mount(<TestComponent trigger={null as any} />); | |
| const wrapper = mount(<TestComponent />); |
| @@ -24,7 +24,7 @@ describe('useTriggerElement', () => { | |||
| }); | |||
|
|
|||
| it('"trigger" can be null', () => { | |||
There was a problem hiding this comment.
| it('"trigger" can be null', () => { | |
| it('"trigger" can be undefined', () => { |
There was a problem hiding this comment.
👍 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.
ecraig12345
force-pushed
the
ts4
branch
5 times, most recently
from
f3b6642
to
a4a552b
Compare
There was a problem hiding this comment.
Explanatory comments for some things now that @joschect is taking over the upgrade
| "packages/web-components/typescript", | ||
| "packages/web-components/typescript/**", | ||
| "packages/web-components/**/typescript", |
There was a problem hiding this comment.
This very thoroughly prevents web-components' different versions of typescript from being hoisted.
- First line: don't hoist direct dep
- Second line: don't hoist deps of typescript (actually not needed since typescript has no deps)
- Third line: don't hoist nested deps on typescript
- This is important because at least two web-components deps have deps on typescript:
@microsoft/api-extractor(TS 3.7.2) and@microsoft/eslint-config-fast-dna(TS 3.9.7)
- This is important because at least two web-components deps have deps on typescript:
| @@ -154,19 +156,12 @@ | |||
| "scripts/package.json" | |||
| ], | |||
| "versionGroups": [ | |||
| { | |||
There was a problem hiding this comment.
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
| @@ -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.
Must be upgraded due to dep on typescript
| @@ -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.
This might have been needed due to the new API Extractor version complaining about unrecognized tags
|
|
||
| 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.
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.
| @@ -27,7 +27,7 @@ | |||
| "@babel/standalone": "^7.10.4", | |||
| "@fluentui/eslint-plugin": "^1.0.1", | |||
| "@mdx-js/loader": "^1.5.5", | |||
| "@microsoft/api-extractor": "7.7.1", | |||
| "@microsoft/api-extractor": "7.13.0", | |||
There was a problem hiding this comment.
This is tied to a specific TS version
| if (requestedIndexIsInPage && this._scrollElement) { | ||
| // We have found the page. If the user provided a way to measure an individual item, we will try to scroll in | ||
| // just the given item, otherwise we'll only bring the page into view | ||
| if (measureItem && this._scrollElement) { |
There was a problem hiding this comment.
By testing this._scrollElement in the parent if we can avoid duplication to avoid confusion on this one, right?
|
Thanks all! @joschect is working on this in a separate branch; closing after talking to @ecraig12345 offline. |
Pull request checklist
$ yarn changeDescription of changes
Upgrade to TypeScript 4.1.3.
So far I didn't include an upgrade of TS in the
@fluentui/web-componentspackage--need to discuss with that team.