-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[DRAFT] Update TypeScript to 4.4.2 #5561
[DRAFT] Update TypeScript to 4.4.2 #5561
Conversation
029fca3
to
c2fa105
Compare
c2fa105
to
7478498
Compare
cd0d125
to
acf92c9
Compare
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.
Thank you so much for go through this and get our code to be ts 4 compatible.
Reviewed 2 of 3 files at r2, 2 of 2 files at r3, 3 of 3 files at r4, 2 of 2 files at r5, 2 of 2 files at r6, 6 of 7 files at r7, 2 of 2 files at r8, 2 of 2 files at r9, 2 of 2 files at r10, 33 of 41 files at r11, 2 of 2 files at r12, 1 of 2 files at r13, 16 of 16 files at r15, 3 of 3 files at r16, 2 of 2 files at r17, 4 of 4 files at r18, 4 of 4 files at r19, 2 of 2 files at r20, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)
tfjs-converter/src/operations/executors/dynamic_executor.ts, line 53 at r20 (raw file):
export const executeOp: InternalOpAsyncExecutor = async( node: Node, tensorMap: NamedTensorsMap, context: ExecutionContext, _resourceManager,
why the _resourceManager starts with _
, should the _resourceManager to have a type?
tfjs-data/src/iterators/microphone_iterator_test.ts, line 34 at r20 (raw file):
const result = await microphoneIterator.next(); expect(result.done).toBeFalsy(); // tslint:disable-next-line:no-any
the any lint ignore
should be removed, also apply for the cases in rest of the file.
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.
Thanks for the review!
I've discovered that this is a breaking change for typescript 3.5 users. When I try to compile an app that depends on tfjs-core and backend-cpu that uses TypeScript 3.5.3, I see the following:
node_modules/@tensorflow/tfjs-core/dist/engine.d.ts:127:9 - error TS1086: An accessor cannot be declared in an ambient context.
127 get backend(): KernelBackend;
~~~~~~~
...
Compiling the downstream app with TS 3.7 and above results in no issues. Should we release this with version 4.0.0?
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)
tfjs-converter/src/operations/executors/dynamic_executor.ts, line 53 at r20 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
why the _resourceManager starts with
_
, should the _resourceManager to have a type?
I put an underscore in front of resourceManager because it's unused, but that's actually against the style guide. Fixed.
I also added its type, although we could actually remove the types from all these function arguments since they are specified by InternalOpAsyncExecutor (although I'm not sure if this would be against the style guide).
tfjs-data/src/iterators/microphone_iterator_test.ts, line 34 at r20 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
the
any lint ignore
should be removed, also apply for the cases in rest of the file.
Done.
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.
I agree if it is breaking for 3.5.x, we should either figure out a work around, or do a major release.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pyu10055)
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.
not sure if this is the solution:
storybookjs/storybook#9463
can you try readonly hackend(): KernelBackend;
?
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pyu10055)
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.
Unfortunately, that solution only works if you're providing manually written declaration files, since it requires editing them.
This might not be a hugely breaking change. Users can upgrade to 3.6.2, which has fewer breaking changes from 3.5.3 than 3.7 has. Alternatively, they can add skipLibCheck: true
to their tsconfigs, which isn't recommended but does fix / hide the issue.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pyu10055)
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.
Reviewed 1 of 1 files at r21.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @pyu10055)
411a8b6
to
19714c4
Compare
Install tslib in the monorepo's root package.json since it's required by the es5 downleveling we do in rollup bundles.
19714c4
to
413ef70
Compare
413ef70
to
cdfcc35
Compare
Superseded by #6346. |
According to the ESModule standard, properties of modules are immutable. TypeScript 4 will enforce this rule. This in particular affects tests for executors in tfjs-converter, in which we often spy on tfOps. This PR removes all instances of spyOn(tfOps,...) and replaces them with a separate spyOps mock / fake which is passed to the executeOp function. It also removes spying on the io module in graph_model_test.ts. This PR was part of the larger TS4 upgrade PR (#6346, #5561), but I'm splitting that PR into pieces that can be merged while we're still using TS3 because it's too large to keep up-to-date. This PR also bumps lib to es2019 in the root tsconfig to allow using Object.fromEntries. This shouldn't affect the code we ship since it's still compiled to the es2017 target. Note that this PR does not bump TypeScript to version 4. It leaves it at 3.
Update TypeScript from 3.5.3 to 4.8.4. Closes #5561, which has gone through review, but was not merged because of breaking changes introduced by typescript 4.4.2.
This PR updates TypeScript from 3.5.3 to 4.4.2. It is still a work in progress:
Tests need to be rewritten to avoid spying on properties of imported objectsThis is done.There's a test failure with TypeScript 4.4.2: This is now fixedThese changes will let us add the
useUnknownInCatchVariables
andnoImplicitOverride
flags to the tsconfig (in a separate PR).To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is