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

Convert shell-client.js to shell-client.ts #10619

Merged
merged 2 commits into from Jul 16, 2019

Conversation

afrokick
Copy link
Contributor

Ok, just tried to convert shell-client to ts.

I have same questions mentioned in #10614 about level branch, VS Code' settings, typedefs.

And I have an issue when compile a file:

mb:tools afrokick$ tsc shell-client.ts
shell-client.ts:112:33 - error TS2345: Argument of type 'Buffer' is not assignable to parameter of type 'string'.
112       const output = JSON.parse(>>Buffer.concat(outputBuffers)<<);

How to properly compile it?

import * as path from "path";
import * as net from "net";
import { isEmacs } from "./utils/utils";
//@ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignored, because:

Could not find a declaration file for module 'chalk'.
'/git/meteor/tools/node_modules/chalk/index.js' implicitly has an 'any' type.
Try `npm install @types/chalk` if it exists or add a new declaration (.d.ts) file containing `declare module 'chalk';`ts(7016)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chalk 2.2.0+ has type definitions file https://github.com/chalk/chalk/releases/tag/v2.2.0

should we update chalk?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m going to keep the require style for now, per #10614 (comment).

@benjamn benjamn force-pushed the refactoring/ts-shell-client branch from d3ed1a2 to f1260a5 Compare July 16, 2019 14:52
@benjamn
Copy link
Contributor

benjamn commented Jul 16, 2019

Here’s where we override the type for JSON.parse to accept Buffers as well as strings: https://github.com/meteor/meteor/blob/release-1.8.2/tools/index.d.ts#L4-L7

If you run

cd path/to/meteor/tools
../meteor npx tsc

instead of just checking the shell-client.ts module in isolation, the index.d.ts file will be picked up, and the errors will go away.

Copy link
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks @afrokick!


this.shellDir = shellDir;
}
constructor(public shellDir: string) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

One of my favorite TypeScript features: you can declare instance variables with appropriate visibility and types right in the parameter list of the constructor!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameter properties)

Could you please provide a feedback in https://forums.meteor.com/t/request-typescript-code-style/49498 about Code Style?

@benjamn benjamn merged commit 2db8d07 into meteor:release-1.8.2 Jul 16, 2019
@afrokick afrokick deleted the refactoring/ts-shell-client branch July 17, 2019 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants