Skip to content
This repository has been archived by the owner on Sep 1, 2021. It is now read-only.

WIP: Migrate to typescript #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JM-Mendez
Copy link
Contributor

As per our conversation this PR starts the migration to typescript.

Summary of changes:

The rpc module has been completely typed, but it still only builds the js source files. Had to leave it like this or else I'd have to continue typing the other packages.

For the types, I used any whenever the compiler complained and there wasn't a type already provided, otherwise it should be a one-to-one mapping.

The only files I didn't migrate were the ones in the root of the module. Doing so would overwrite the original js files. But since they just one-liners that point to the build directory, I don't think it would matter if left alone.

I ran into an issue with the IPC method in the node-ipc package that was being used, but was not exposed in the typings. I used the patch-package module to provide an easy patch instead of forking or maintaining one.

TS 3.0 also introduced the concept of references, which allows building multiple projects. This negated the need for a build script, but it no longer shows progress bars.


I'll wait to hear from you before proceeding with the rest of the files.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 31, 2018
@aaronabramov
Copy link
Contributor

this is awesome! a few notes:

  1. instead of migrating the current package, can you instead put new code into a separate rpc-ts package? when we're done with migration and make sure that all things are working we'll be able to remove rpc and rename rpc-ts back to rpc

  2. can you declare a simple typed package instead of using patch-package? Not sure how TS does it. in flow you can just declare module 'node-ipc' { ... }, i imagine ts has something similar. using an extra package and adding a postinstall step seems like a huge overhead.

  3. The .js files in the root of the package are used to export flow definitions as part of the package.
    so in my other products i can just require('@jest-runner/electron') and it has all types included out of the box. what's the typescript analog for it?

@aaronabramov
Copy link
Contributor

also we'll probably need to add some infra for writing tests in ts

@JM-Mendez
Copy link
Contributor Author

JM-Mendez commented Dec 31, 2018

can you declare a simple typed package instead of using patch-package?

I can't merge declarations since I need access to an unexported type. The only other way is to copy the typings file into a local directory and edit and maintain that, or declare the module casting everything to any. Below is the relevant section of the @types/node-ipc package

declare namespace NodeIPC {
  class IPC {
    // ... rest of namespace
}

// 1. I need access to `RootIPC.IPC`, but `NodeIPC.IPC` is not exported
declare const RootIPC: NodeIPC.IPC & {IPC: new () => NodeIPC.IPC};

declare namespace RootIPC {
// 2. so I have to export `NodeIPC.IPC` here, and in a namespace since
// this is a commonjs module
  export type IPC = NodeIPC.IPC;
}

export = RootIPC;

The .js files in the root of the package are used to export flow definitions as part of the package.
so in my other products i can just require('@jest-runner/electron') and it has all types included out of the box. what's the typescript analog for it?

When using the references setup typescript builds declaration files for each source file, alongside the transpiled source. So the types will also be imported when requiring the packages. I believe this accomplishes your goal. If you go this route there will be some lines that will have to change like:

// electron_process_injected_code.js

// import RPCConnection from '@jest-runner/rpc/RPCConnection';
import RPCConnection from '@jest-runner/rpc/src/RPCConnection';

// ...
  const rpcConnection = new RPCConnection(JestWorkerRPC);
  await rpcConnection.connect();
});

also we'll probably need to add some infra for writing tests in ts

migrating the tests to typescript should be fairly straightforward as well. Adjusting the types and using ts-jest should do most of the work. I'll do that after the modules are typed.

@aaronabramov
Copy link
Contributor

we can cast it to any for now! and then decide what to do with it later :)
other than that it looks good!

@JM-Mendez JM-Mendez force-pushed the typescript branch 2 times, most recently from 9be7a2e to 22af71a Compare January 7, 2019 07:53
@JM-Mendez
Copy link
Contributor Author

JM-Mendez commented Jan 7, 2019

Ok, I finally managed to conquer babel and get the rpc module migrated to typescript. For some reason it would not work with .babelrc and only with babel.config.js. Also, I'm not sure what transform-inline-imports-commonjs does in the build script. Apparently it doesn't work with babel 7, so I just commented it out for now.

For the time being, it will generate correctly the required files, but I set up a test file in the electron directory, and I guess make it the official file once it's been migrated as well.

As for the tests, they all pass on my machine, but I can't seem to update the snapshots on the ci server, and those are the only 2 tests failing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants