Add an initial draft for a Resin TypeScript skeleton project. #1

hedss · 2017-02-01T17:50:53Z

No description provided.

hedss · 2017-02-01T17:52:14Z

To go along with the PR review for resin-io/hq#612.
This is a draft proposal, so lots of comments required.

dfunckt · 2017-02-01T17:59:29Z

gulpfile.js

+const tsProject = typescript.createProject('tsconfig.json');
+
+const OPTIONS = {
+	header: true,


@hedss you preach spaces but use tabs? :p

I use an editor which decides what to use based on what it sees first. And I think I transformed this originally from a CS file, so that's why. This is why we have reviews. ;-)

pimterry

Just to continue the tabs/spaces debate - this PR uses tabs (Gulpfile), 4 spaces (implement-me.ts) and 2 spaces (package.json). Argh.

Looks like we don't have resin-tslint yet, which would help with the ts part. We could manage indentation with an editorconfig too/instead? That way we cover everything, not just the typescript bits.

pimterry · 2017-02-01T17:59:17Z

package.json

+  ],
+  "author": "Heds Simons <hedley@resin.io>",
+  "license": "Apache-2.0",
+  "main": "./build/implement-me.js",


If this file starts out as lib/index.ts instead, you can clone this project and just start editing it and immediately have something that's ready to go. As implement-me.ts/js, your first step for every new project has to be going through and doing a find/replace everywhere - we could just skip that 😄

pimterry · 2017-02-01T18:02:50Z

gulpfile.js

+		.pipe(sourcemaps.write('./', { includeContent: false,
+			sourceRoot: '../lib',
+			// There is currently an issue where not doing this won't generate
+			// correctly pathed mapfiles.


Is there a github issue for this somewhere? Would be nice to link here, so we can kill this once it's sorted.

I've actually found the reason for this. There's disparity between the inlining of source maps and creating them externally. I suspect it's 'not a bug' as selecting both options is a user problem. However, it now works correctly without in this and Procbots, so I'm removing it.

pimterry · 2017-02-01T18:09:47Z

package.json

+  "description": "Skeleton template for a Resin TypeScript project",
+  "homepage": "https://github.com/resin-io/resin-typescript-skeleton#readme",
+  "preferGlobal": true,
+  "types": "build/**/*.d.ts",


Do wildcards work here? I hadn't really thought this through before, but looking at it now this surprises me a bit. I think types is supposed to point to the types of the root main module (so currently ./build/implement-me.d.ts). As main maps require('my-module') to a specific script, types maps require('my-module') to the module's full type definition.

If you want any of the other definitions, you import them by hand as you would for a specific file in any other module (import { GithubAction } from 'procbots/build/githubbot-api), or your root type definition can export them explicitly (import { GithubBot, GithubAction } from 'githubbot'), if they're a core part of the external API.

I'll need to check this. I actually found this on a TS thread with a VisualCode developer specifying it. Will need to try and re-find that thread.

Can't find, but yeah, I concur. :)

pimterry · 2017-02-01T18:10:38Z

package.json

+  "version": "0.0.1",
+  "description": "Skeleton template for a Resin TypeScript project",
+  "homepage": "https://github.com/resin-io/resin-typescript-skeleton#readme",
+  "preferGlobal": true,


I don't think this should preferGlobal.

pimterry · 2017-02-01T18:16:13Z

tsconfig.json

+		"sourceMap": true,
+		"target": "es5",
+		"noUnusedParameters": true,
+		"noUnusedLocals": true


This doesn't specify an output directory. Does it do ./build/ by default?

It does, and that's part of the gulpfile. You'll notice that the .vscode/tasks.json species that the build occurs via gulp, hence this is taken care of. You can build either via the commandline gulp build, or via VS Code. Other editors will need to be set up as required.

Imo, as a general rule it would be nice to keep TS build config all in the tsconfig, as much as possible, instead of adding extra parts of it only in gulp. Obviously for cases where we can't then gulp is fine, but for the output directory at least we certainly can.

Every tool that understands typescript understands tsconfig, but not all of them will happily delegate the build process to gulp. E.g. wallaby, many other non-VS code editors. If everything required is in tsconfig, we shouldn't need to do any special setup for other editors at all - typescript support is the only prerequisite. Plus having one canonical place for TS build config is convenient for finding and managing it.

pimterry · 2017-02-01T18:20:52Z

package.json

+    "build": "gulp build"
+  },
+  "devDependencies": {
+    "@types/node": "^6.0.48",


Fun open question (this should probably make it into the HQ guide): should @types dependencies be in devDependencies or normal dependencies?

TypeScript guide says dependencies for libraries, so downstream TS projects get them, but devDependencies if this project is an end-user application/command-line tool/etc, where there is no downstream.

I've taken this to be a CLI, hence in devDependencies. I think this should therefore be in the README.md as a 'How to alter this project depending on use case`, maybe.

jhermsmeier · 2017-02-01T19:39:42Z

.vscode/launch.json

+{
+    // Use IntelliSense to learn about possible Node.js debug attributes.
+    // Hover to view descriptions of existing attributes.
+    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387


I think this comment can be removed – it's invalid JSON, and also doesn't look like it's content is crucial to kicking off a new project ;)

It's not, it's boilerplate VS Code adds as standard.

jhermsmeier · 2017-02-01T19:44:40Z

LICENSE

@@ -0,0 +1,201 @@
+                                 Apache License


General curiosity question: Is there a specific reason we use the Apache License by default, instead of, say MIT – or does it just come down to personal preference?

There's a thread in flowdock which I can't find as it's not on the process channel where I expected it.

@lekkas There was a discussion about this recently, can you remember the ins and outs?

@hedss (better late than never I guess) I can't recall being part of this discussion tbh, so can't really enumerate ins and outs

jhermsmeier · 2017-02-01T19:58:41Z

What do you think about adding an .editorconfig, to make keeping indentation, line endings and such consistent automatically?

For Example:

# editorconfig.org
root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

[Makefile]
indent_style = tab

[*.{md,mkd,markdown}]
trim_trailing_whitespace = false

[*.ts]
indent_size = 4

hedss · 2017-02-02T09:36:36Z

@jhermsmeier I think an .editorconfig is a very good idea. Let's sort out the TS style conversation and then I'll add one based on the result.

* `index.ts` is now the default source file, for fast project pickup * Update `README.md` to ensure that `@types` location in `package.json` is clarified * Update `gulpfile.js` to remove now unrequired sourcemap change * Ensure 4 spaces throughout

CameronDiver · 2017-02-02T12:28:35Z

.vscode/launch.json

@@ -0,0 +1,26 @@
+{


@hedss I'm not convinced we should have a .vscode directory in the repositories of projects themselves, it seems awfully programmer specific. It makes sense to maybe have it in this repo as an example, but I'm not sure it should actually be added to typescript project repos.

I agree in the sense that it's not specific to TS. On the other hand, there's some stuff here that makes developing in it way easier (and I know quite a few of us are now using it), and feeling some of this stuff out was a pain.
Need more opinions from others, please!
Should this maybe go in the TS standard instead? (Regardless, it has to be documented somewhere to help others.)

I'm cautiously in favour of it: it doesn't cause many problems for any non-vsc users, especially since it's a hidden file so you often even see it, and it makes life much nicer for vsc users, which I'm expecting that to be most people. I don't have strong views here though.

I fully agree that having it (at least) here as an example is definitely what we should do. I don't know it just feels wrong having it in the repo, but then I guess it comes down to the same thing that including build output does, convenience.

I think.. I'm going to keep it in for the moment. If there are complaints, we can review it later.

I don't think we should have any editor/IDE specific configuration in our repos except maybe something like http://editorconfig.org . We shouldn't even put editor specific things in .gitignore in this repo. Each developer should have their own global gitignore file containing the irrelevant files of their specific setup

pimterry · 2017-02-02T12:28:40Z

gulpfile.js

+        .pipe(tsProject()).on('error', gutil.log)
+        .pipe(sourcemaps.write('./', {
+			includeContent: false,
+            sourceRoot: '../lib'


Related to my comment out the build output dir, the typescript compiler can do sourcemaps for you too. Do we need to do this separately ourselves? You should be able to configure internal/external, location, source roots and content inclusion all from normal tsconfig params.

Yep, I'll look at removing these requirements from the gulpfile if possible.

I've had to alter slightly how the gulpfile deals with hand-made declaration files to copy them across.

It now uses options from the tsconfig.json.

* gulp build now uses the sources from the `tsconfig.json` file

petrosagg · 2017-03-14T22:55:13Z

.vscode/launch.json

@@ -0,0 +1,26 @@
+{


I don't think we should have any editor/IDE specific configuration in our repos except maybe something like http://editorconfig.org . We shouldn't even put editor specific things in .gitignore in this repo. Each developer should have their own global gitignore file containing the irrelevant files of their specific setup

petrosagg · 2017-03-14T23:05:56Z

package.json

+        "resin",
+        "typescript"
+    ],
+    "author": "Heds Simons <hedley@resin.io>",


Should we put a placeholder here or maybe omit this altogether?

petrosagg · 2017-03-14T23:06:21Z

package.json

+    "main": "lib/index.ts",
+    "repository": {
+        "type": "git",
+        "url": "git+https://github.com/resin-io/resin-typescript-skeleton.git"


petrosagg · 2017-03-14T23:06:26Z

package.json

+        "url": "git+https://github.com/resin-io/resin-typescript-skeleton.git"
+    },
+    "bugs": {
+        "url": "https://github.com/resin-io/resin-typescript-skeleton/issues"


pimterry · 2017-03-17T10:19:37Z

A quick note on @petrosagg's last 3 comments: if we do wrap this up with Yeoman (as somebody mentioned in flowdock), we could very easily do dynamic config that'd make swapping out package.json values extremely easy. I'd just blank those out for this PR I think, but it'd be a good extension later.

pimterry

Now LGTM, except for petros's last comments. I'd mildly like to include vscode config, since it's project specific and it'll be relevant to nearly all devs here, but I can see the argument to drop it, and don't really mind that either.

CameronDiver · 2017-03-17T10:26:52Z

package.json

+    "version": "0.0.1",
+    "description": "Skeleton template for a Resin TypeScript project",
+    "homepage": "https://github.com/resin-io/resin-typescript-skeleton#readme",
+    "types": "build/index.d.ts",


I think that this should be "types": "src/index.ts". It's what myself and @Page- have been using and it makes the most sense IMO. Firstly .d.ts files do not need to be generated (assuming index.d.ts wasn't handcrafted) and the types header in package.json is only useful when using typescript anyway, implying there is a typescript compiler available. In addition it feels much cleaner. A good example of this is https://github.com/resin-io/resin-docker-build and https://github.com/resin-io-modules/resin-bundle-resolve and these two modules have been consumed happily from JS and TS with minor setup.

@pimterry Thoughts on this, please. It's strongly against the way MS recommend you do this, types should point to a declaration file and not a source file:

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

Recommendation for exposing multiple TypeScript modules from single NPM package microsoft/TypeScript#8305

Sorry, I don't have any hard answers here. I've always seen declaration files used for types, but if the non-declaration file approach works the same too, but with fewer files and less config, that sounds great to me.

Personally, I'd probably go for @CameronDiver's approach for now. If we do find there's a good reason why this won't quite work then it isn't very hard to change later, and otherwise I do agree it's cleaner.

* Empty strings for values to be filled in in `package.json` * Add a `.editorconfig` file default * Remove Visual Code project files

CameronDiver

Apart from the .vscode directory I think everything else looks fine.

For reference I think it's not a good idea to be adding a .vscode to any repository, much less a skeleton which people will build from.

Add an initial draft for a Resin TypeScript skeleton project.

dd9c8b3

hedss requested review from dfunckt, Page-, emirotin, petrosagg, brownjohnf, pimterry, lekkas and CameronDiver February 1, 2017 17:56

dfunckt reviewed Feb 1, 2017

View reviewed changes

pimterry suggested changes Feb 1, 2017

View reviewed changes

jhermsmeier reviewed Feb 1, 2017

View reviewed changes

Modify skeleton post-peer review.

bf2adcb

* `index.ts` is now the default source file, for fast project pickup * Update `README.md` to ensure that `@types` location in `package.json` is clarified * Update `gulpfile.js` to remove now unrequired sourcemap change * Ensure 4 spaces throughout

CameronDiver reviewed Feb 2, 2017

View reviewed changes

pimterry reviewed Feb 2, 2017

View reviewed changes

hedss added 2 commits February 2, 2017 13:35

Update config files based on recommendations.

011b278

* gulp build now uses the sources from the `tsconfig.json` file

Ensure gulp file uses configs set in tsconfig.json.

b6e08d8

petrosagg reviewed Mar 14, 2017

View reviewed changes

pimterry approved these changes Mar 17, 2017

View reviewed changes

CameronDiver suggested changes Mar 17, 2017

View reviewed changes

Update based on peer review.

5a4caaa

* Empty strings for values to be filled in in `package.json` * Add a `.editorconfig` file default * Remove Visual Code project files

CameronDiver approved these changes Mar 22, 2017

View reviewed changes

hedss merged commit 8757ada into master Mar 23, 2017

hedss deleted the initial-wip branch March 23, 2017 10:15

Add an initial draft for a Resin TypeScript skeleton project. #1

Add an initial draft for a Resin TypeScript skeleton project. #1

Conversation

hedss commented Feb 1, 2017

hedss commented Feb 1, 2017

Choose a reason for hiding this comment

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

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

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

jhermsmeier commented Feb 1, 2017

hedss commented Feb 2, 2017

CameronDiver Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

hedss Feb 2, 2017 • 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

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

pimterry commented Mar 17, 2017

pimterry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedss Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CameronDiver left a comment

Choose a reason for hiding this comment

CameronDiver Feb 2, 2017 •

edited

Loading

hedss Feb 2, 2017 •

edited

Loading

hedss Mar 22, 2017 •

edited

Loading