convert to typescript #18

emirotin · 2017-07-19T23:13:46Z

Change-Type: minor

This should actually be good enough for a PR but I'm going to move the relevant parts to resin-karma-config and typed-error repos

Page-

It would really help to split this up into separate commits, rather than having a load of changes bundled into one

Page- · 2017-07-20T00:50:12Z

tsconfig.json

+    "declaration": true
+  },
+  "include": [
+		"lib/**/*.d.ts",


Inconsistent indentation here

Page- · 2017-07-20T00:50:48Z

package.json

@@ -3,6 +3,7 @@
  "version": "2.7.0",
  "description": "Collection of Resin.io JavaScript errors",
  "main": "build/errors.js",
+  "types": "build/errors.d.ts",


Any reason for not pointing this at the typescript file instead?

As of now we're not publishing the lib directory altogether.

Really? Is there a good reason for that? I think our standard approach everywhere else is to publish the lib with the TS, point types at that directly, and skip generating type definitions. I don't know of any downsides, and it's definitely simpler.

At least our styleguide mentions the .d.ts file. Will update to include the TS sources and point at them.

Page- · 2017-07-20T00:51:31Z

package.json

+    "build": "rimraf ./build && npm run prettify:src && tsc && npm run prettify:dist",
+    "prepublish": "npm run build && npm run readme && npm run test",
+    "prettify:src": "prettier --use-tabs --single-quote --write lib/*.ts",
+    "prettify:dist": "prettier --use-tabs --single-quote --write build/*.js",


Why do you run prettifier on the generated code? It'd be better imo to leave it as is (rather than risk messing up the sourcemaps)

Good point about the sourcemaps, will remove

Page- · 2017-07-20T00:51:56Z

package.json

+    "resin-config-karma": "^1.0.1",
+    "rimraf": "^2.6.1",
+    "tslint": "^5.5.0",
+    "typescript": "2.4.2"


Why do you target a fixed version?

Explained in a diff PR

Page- · 2017-07-20T00:52:30Z

package.json

-    "prepublish": "gulp build",
+    "test:node": "mocha --reporter min tests/**/*.spec.js",
+    "test:browser": "karma start",
+    "test": "tslint lib/**/*.ts && npm run build && npm run test:node && npm run test:browser",


Doesn't tslint use tsconfig to see what to target?

I don't know, will check

Nope, complaints about the missing files without it

Page- · 2017-07-20T00:53:49Z

appveyor.yml

@@ -11,14 +11,14 @@ cache:
 # what combinations to test
 environment:
  matrix:
+    - nodejs_version: ''


What does this do?

Targets the most recent version (8 as of now)

Is this equivalent to stable, which also resolves to 8 right now? Slightly clearer that way if so.

LucianBuzzo

LGTM apart from page's comments

pimterry · 2017-07-21T11:06:21Z

appveyor.yml

@@ -11,14 +11,14 @@ cache:
 # what combinations to test
 environment:
  matrix:
+    - nodejs_version: ''


Is this equivalent to stable, which also resolves to 8 right now? Slightly clearer that way if so.

pimterry · 2017-07-21T11:08:36Z

lib/errors.ts

+	public exitCode: number;
+}
+ResinError.prototype.code = 'ResinError';
+ResinError.prototype.exitCode = 1;


Is there a reason we don't do this (here and in all the cases below) inline in the class definition?

public code = 'ResinError'?

It used to be on the prototype, this would make it the instance prop. I don't mind, but decided to keep it unchanged.

I did not realise that, but you're totally right: microsoft/TypeScript#1250

Fair enough, in that case this is fine by me 👍

pimterry · 2017-07-21T11:09:52Z

package.json

@@ -3,6 +3,7 @@
  "version": "2.7.0",
  "description": "Collection of Resin.io JavaScript errors",
  "main": "build/errors.js",
+  "types": "build/errors.d.ts",


Really? Is there a good reason for that? I think our standard approach everywhere else is to publish the lib with the TS, point types at that directly, and skip generating type definitions. I don't know of any downsides, and it's definitely simpler.

emirotin · 2017-07-24T22:20:57Z

karma.conf.js

+const packageJSON = require('./package.json')
+
+module.exports = (config) => {
+	const karmaConfig = {


I will later refactor this into a version2 of the resin-config-karma module, for now it's just faster to experiment here.

emirotin · 2017-07-24T22:40:35Z

FYI now we have the tests here running in headless Chrome locally, in Travis, and in AppVeyor, with webpack and babel used for tests packaging.
I will squash the commits, move the new karma config to the resin-config-karma, and use v2 of the typed-error before merging, of course.

emirotin · 2017-07-24T22:51:54Z

Also @Page- should we start using VB for this repo?

Page- · 2017-07-25T00:15:29Z

@emirotin I don't see why not, but I've never touched this repo before so probably not the best to decide

emirotin · 2017-07-25T06:40:57Z

OK

…

On Tue, Jul 25, 2017 at 3:15 AM, Page- ***@***.***> wrote: @emirotin <https://github.com/emirotin> I don't see why not, but I've never touched this repo before so probably not the best to decide — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgGCFbv2sgu_gnhvsTZHdcKDMhQC18kks5sRTOhgaJpZM4Odadj> .

-- Eugene Mirotin Senior Frontend Engineer site: Resin.io <https://resin.io/>, twitter: @resin_io <https://twitter.com/resin_io>

emirotin · 2017-07-25T19:39:06Z

@pimterry @Page- please re-review

pimterry · 2017-07-26T09:36:24Z

lib/errors.ts

+ * @example
+ * checkId = (id) ->
+ * 	if typeof id isnt 'number'
+ * 		throw new errors.ResinInvalidParameterError('id', id)


This example probably shouldn't be coffeescript any more either

pimterry · 2017-07-26T09:37:44Z

package.json

+    "lint": "tslint lib/**/*.ts",
+    "test": "npm run lint && npm run build && npm run test:node && npm run test:browser",
+    "build": "rimraf ./build && npm run prettify && tsc && npm run readme",
+    "prepublish": "npm run build && npm run test",


test already runs build, so you don't need both here

pimterry · 2017-07-26T09:53:57Z

tsconfig.json

+    "noUnusedParameters": true,
+    "preserveConstEnums": true,
+    "pretty": true,
+    "sourceMap": true


I know the guidelines don't include it as standard (though I've just filed a PR), but we should really include "strictNullChecks": true here too.

pimterry · 2017-07-26T09:54:32Z

tests/errors.spec.js

@@ -0,0 +1,34 @@
+var m = require('mochainon')


Why is this file not TS?

Some tests should actually be in JS as TS won't let you go some wrong paths. But this one can be TS indeed.

Ah, ok, interesting point, that adds some useful context to the other discussion, thanks!

You should be able to get round that, nice and explicitly, with some good use of <any>, and I think probably most tests should be doing things that are technically valid at a type level, because that's where most interesting testing happens. I agree there's other interesting cases though, and you're right that it's a little fiddly, but I think ignoring it until we hit problems and going for TS by default is nicer, imo.

I'm going to enable both TS and JS tests in the karma config and convert this one to TS then

Sounds good to me

emirotin · 2017-07-26T22:07:10Z

Hey @pimterry can you help me here please? I'm trying to convert the tests to TS and having

> [email protected] test:node:ts /home/guard/work/resin-errors
> mocha -r ts-node/register --reporter min tests/**/*.spec.ts


/home/guard/work/resin-errors/node_modules/ts-node/src/index.ts:307
        throw new TSError(formatDiagnostics(diagnosticList, cwd, ts, lineOffset))
              ^
TSError: ⨯ Unable to compile TypeScript
tests/errors.spec.ts (4,20): Could not find a declaration file for module 'mochainon'. '/home/guard/work/resin-errors/node_modules/mochainon/build/mochainon.js' implicitly has an 'any' type.
  Try `npm install @types/mochainon` if it exists or add a new declaration (.d.ts) file containing `declare module 'mochainon';` (7016)
tests/errors.spec.ts (12,18): Element implicitly has an 'any' type because type 'typeof "/home/guard/work/resin-errors/lib/errors"' has no index signature. (7017)
    at getOutput (/home/guard/work/resin-errors/node_modules/ts-node/src/index.ts:307:15)

These are two different errors.

It cannot find the typings for mochainon lib despite the fact I've added them and a ref to them from the test file
It doesn't like the dynamic nature of the test even though I've cast the error class to the proper type

pimterry

I've added a bunch of notes on why the TypeScript wasn't working, just for background info, and then also pushed a fix too since I wrote it out anyway sorting this.

pimterry · 2017-07-27T11:38:41Z

tests/mochainon.d.ts

+}
+
+declare const mochainon: Mochainon;
+export default mochainon;


Ok: this isn't a type definition, it's an actual module, which is why it's not successfully defining 'mochainon' for your test file. If you have an import or an export at the top-level, you're a module, and modules aren't allowed to do declare statements at all (I'm not sure why it isn't in the compiler output with the other errors, but for me in VSC I get an error on the declare at the top here).

You don't want this. You want a module declaration: to do that, add a body to your declare module 'mochainon' statement, that defines the contents of that module. That's pretty close to what you've got here (but with braces around the whole lot).

There's on other bug here: you've used export default, but it's not actually a default export (require('mochainon').default doesn't exist) - it should be export = mochainon.

I've actually tried this approach first and got the VSC error

pimterry · 2017-07-27T11:41:48Z

tests/errors.spec.ts

@@ -0,0 +1,43 @@
+/// <reference path="mochainon.d.ts" />


Generally you never ever want to use ///<reference....

It's a leftover from before TypeScript had a proper module system. You either want to have an actual dependency TypeScript can understand and follow to get the type (e.g. a normal import), or you want to include the type definition in your tsconfig.json. Here the problem is just that your tsconfig.json only matches lib/**/*.d.ts, and it also needs test/**/*d.ts (or you could have a single top-level typings folder).

It's a little weird that this is lib and test together - in principle you could have test and prod tsconfigs, but I don't think it's worth it personally.

Again this is a leftover of my desperation to fix it. I wanted to keep this scoped to the tests only indeed, and it's impossible to import the typings otherwise if the module is already imported. But I'm fine with the current solution.

pimterry · 2017-07-27T12:01:35Z

tests/errors.spec.ts

+const runForAllErrors = (fn: (errorName: string, ErrorClass: ResinErrorClass) => void) => {
+	for (const errorName in errors) {
+		if (errors.hasOwnProperty(errorName)) {
+			fn(errorName, errors[errorName] as ResinErrorClass);


This is upset because errorName is just a string, and errors doesn't have an index signature, so it has no idea what the result of searching it for a string is (what's the type of errors[unknownString]?)

The missing info it needs is that errorName is a key of errors - annoyingly for/in isn't enough to know that in general: microsoft/TypeScript#12314.

We are pretty sure of that ourselves though, so we can just cast it: errors[errorName as keyof typeof errors].

That works, but leaves you with two casts (this new key cast, and then the ResinErrorClass cast as well). That's a little messy, but it's not actually safe to remove as ResinErrorClass, because that isn't actually true. Part of the type of ResinErrorClass is that it has a constructor which takes an error as the first argument. Lots of these error classes don't have that, so don't match the type. Type hierarchies of class objects (as opposed to instances) are weird. What you want to say it that it's an object with the same prototype, not that it's literally a same-shaped class (because it's not).

emirotin · 2017-07-27T12:07:08Z

tests/mochainon.d.ts


-import * as chai from 'chai';
-import * as sinon from 'sinon';
+	import * as chaiImport from 'chai';


chaiImport is unused?

That's weird, sorry, not sure how that ended up there. Or why that works...

Following the references, chai has a top-level declare const chai: ..., which I think actually declares that as a global type, so maybe you don't even need this. Presumably that's because it gets injected globally in a lot of normal chai usage? Feels weird though, I think we should keep importing it explicitly.

emirotin · 2017-07-27T14:58:14Z

Hm. Some sub-dependency adds Symbol to the bundle, need to figure out how to transpile the entire test bundle to safe ES5. Tried not excluding node_modules but didn't help at first attempt.

emirotin · 2017-07-27T21:15:46Z

This is super weird.
I've configured Karma to run my TS tests.
Now they work (locally) if I ran them as node run test:browser.
The Node.js tests work too
But iff I run the tests as npm test, or directly npm run test:node && npm run test:browser THEN I'm seeing the old error about the missing type def for mochainon. How can this be possible?

UPD1: running the seemingly equivalent npm run test:node:js && npm run test:node:js && npm run test:browser works fine o_O

UPD2: seems to be stochastic now, not always reproducible. Super weird.

emirotin · 2017-07-28T08:21:32Z

So here's the current state of the testing problems:

nondeterministic TS error about the missing typings - despite them being present and the awesome-ts-loader using the local tsconfig
somewhere in the bundle there's Symbol that makes the older browsers choke. And I can't figure out where it is
The new one. I keep not compiling the node_modules but the current AppVeyor builds fail with the messages that I'd got when I did compile them. The deoptimisation warning is specifically the symptom of that.

pimterry · 2017-07-31T11:32:44Z

nondeterministic TS error about the missing typings - despite them being present and the awesome-ts-loader using the local tsconfig

I can't seem to reproduce this at all, running this locally - for me it passes 100% of the time. Are you seeing this locally, or in CI somewhere?

somewhere in the bundle there's Symbol that makes the older browsers choke. And I can't figure out where it is

If you change the browser runner to Chrome, and hit the debug button in the window that comes up, you get a page with the tests running, and all the webpack output loaded and examinable. From there it's fairly obvious if you find the chai-as-promised output: babel converts this for/or into a loop using Symbol.iterator().

That's the ES2015 plugin, which says that to use iterators you have to include a polyfill. We'll either want to pull in a polyfill during testing (fine by me), or find some other plugin setup with better backward compatibility.

I keep not compiling the node_modules but the current AppVeyor builds fail with the messages that I'd got when I did compile them. The deoptimisation warning is specifically the symptom of that.

No idea why that compilation error is only happening in AppVeyor, but it looks like it's just looking for the es3 preset for some reason, which should be very solvable. Have you tried just including babel-preset-es3?

emirotin · 2017-07-31T11:45:36Z

Thanks for looking into this. I can repro the typings error locally like every 3rd or 4th time, but I think I can ignore it for now.

Symbol from the for-of makes sense, going to add the polyfill for now and check other options later.

No, I didn't try adding ES3 config as it may mask a more serious problem, specifically builds working differently on Windows. I'll look into it deeper before resorting to that fix.

emirotin · 2017-08-01T06:52:55Z

1 - still there but probably doesn't fail the build

2 - solved with the polyfill

3 - it's because of the posix-specific forward slash in the exclusion regexp for babel-loader. Fixing now

emirotin · 2017-08-01T07:46:26Z

All tests passing! @pimterry can you approve now?

pimterry · 2017-08-01T13:58:30Z

tests/errors.spec.js

@@ -0,0 +1,38 @@
+const m = require('mochainon')


This file can go now, since we've got the TS tests working.

pimterry · 2017-08-02T11:19:33Z

package.json

-    "prepublish": "gulp build",
+    "test:node:js": "mocha --reporter spec tests/**/*.spec.js",
+    "test:node:ts": "mocha -r ts-node/register --reporter spec tests/**/*.spec.ts",
+    "test:node": "npm run test:node:js && npm run test:node:ts",


Now we've got the TS tests working happily, we can simplify this

pimterry

👍 👍 👍

Change-Type: minor

emirotin · 2017-08-02T14:56:24Z

To ensure backward compatibility I've created an SDK branch with this branch of errors. SDK tests make lots of assertions for the resin-errors error messages, all seem to be passing fine.
balena-io/balena-sdk#359

So, going to merge now.

emirotin · 2017-08-02T15:02:08Z

Published as 2.8.0

emirotin requested review from Page-, pimterry and LucianBuzzo July 19, 2017 23:13

Page- requested changes Jul 20, 2017

View reviewed changes

LucianBuzzo reviewed Jul 20, 2017

View reviewed changes

pimterry suggested changes Jul 21, 2017

View reviewed changes

emirotin commented Jul 24, 2017

View reviewed changes

pimterry suggested changes Jul 26, 2017

View reviewed changes

Page- approved these changes Jul 26, 2017

View reviewed changes

pimterry reviewed Jul 27, 2017

View reviewed changes

emirotin commented Jul 27, 2017

View reviewed changes

pimterry suggested changes Aug 2, 2017

View reviewed changes

emirotin force-pushed the typescript branch from a7e6673 to 31cc675 Compare August 2, 2017 13:12

pimterry approved these changes Aug 2, 2017

View reviewed changes

convert to typescript

8f6e6d3

Change-Type: minor

emirotin force-pushed the typescript branch from 1ff1a64 to 8f6e6d3 Compare August 2, 2017 14:00

emirotin merged commit e98e92c into master Aug 2, 2017

emirotin deleted the typescript branch August 2, 2017 14:56

convert to typescript #18

convert to typescript #18

Conversation

emirotin commented Jul 19, 2017

Page- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianBuzzo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emirotin commented Jul 24, 2017

emirotin commented Jul 24, 2017

Page- commented Jul 25, 2017

emirotin commented Jul 25, 2017 via email

emirotin commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emirotin commented Jul 26, 2017

pimterry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emirotin commented Jul 27, 2017

emirotin commented Jul 27, 2017 • edited Loading

emirotin commented Jul 28, 2017

pimterry commented Jul 31, 2017

emirotin commented Jul 31, 2017

emirotin commented Aug 1, 2017

emirotin commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimterry left a comment

Choose a reason for hiding this comment

emirotin commented Aug 2, 2017

emirotin commented Aug 2, 2017

LucianBuzzo left a comment •

edited

Loading

emirotin commented Jul 27, 2017 •

edited

Loading