Skip to content

Set up build-time code for TypeScript development #288

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

Merged
merged 10 commits into from
Sep 21, 2018

Conversation

dfreeman
Copy link
Member

@dwickern @jamescdavis @mike-north

Overview

This is extracted from prototyping I've been doing with Babel 7 off and on. At a high level, it enables us to begin writing addon node-land code in TypeScript. In development, TypeScript files are loaded via ts-node, whereas in CI and in the published package, the transpiled JS is used instead.

This doesn't fully supercede #35, as it doesn't actually convert any meaningful amount of code to TS (just one small utility to ensure everything works end-to-end), but it should make it simple to do so in the future. As mentioned in #287, if we adopt the Babel plugin for managing transpilation, much of our existing code will no longer be needed, so converting that likely wouldn't be a wise time investment. However, this will allow us to write all new code with TS and convert what remains (e.g. the default blueprint and commands) when time permits.

I've aimed to break the change into a set of self-contained commits that build on one another in a way that's easy to review.

Open Questions

  • What kind of linting do we want for .ts files? Currently there's none set up, but the two immediate options I see are TSLint or ESLint + typescript-eslint-parser and maybe eslint-plugin-typescript. There seems to be a bit of a leaning toward the latter in the Ember + TS community at this point (and I know Chris is an advocate of that approach), but do y'all have strong opinions?

  • Where should the source and transpiled output live? Most of the appealing options for input directory names already have meaning to Ember CLI (e.g. addon and src), so I took a cue from ember-auto-import and used ts for the input and js for the output. These names are nice and terse, but without the force of being any kind of established convention so far, they don't communicate a lot of information. Any better suggestions?

  • Does the behavior in our index.js entry point represent our ideal ergonomics? Currently the code in index.js will prefer transpiled output over the source .ts files if it's present. This could lead to a pretty frustrating DX if one of us happens to have files sitting around in js/ and doesn't realize it. I could imagine either:

    • giving preference to loading the TypeScript source instead
    • issuing a warning when isDevelopingAddon is true but we're running from a .js file

    In principle either of these approaches could work, since the .ts files wouldn't be present when testing in CI or production (the ts/ directory is deleted before running tests in CI and excluded from the tarball that gets pushed to npm).

@jamescdavis
Copy link
Member

  • What kind of linting do we want for .ts files?

    We are successfully using both TSLint and ESLint + typescript-eslint-parser + eslint-plugin-typescript in ember-osf-web, but, if I had to pick one, I would go with ESLint. I'd like to suggest extending airbnb-base and ember/recommended. If we use TSLint also/instead, I'd like to recommend extending tslint:recommended, tslint-consistent-codestyle, and tslint-eslint-rules. We have decent rulesets in ember-osf-web for both that we could use as a starting place. I'll also go ahead and vote for 4 space indentation, but that's not a hill I'm willing to die on 😉. I have lots of strong opinions, weakly held on linting and code style if anybody cares to hear them 😆.

  • Where should the source and transpiled output live?

    Maybe ts_src and js_out?

  • Does the behavior in our index.js entry point represent our ideal ergonomics?

    I'd prefer issuing a warning (perhaps even an error, maybe configurable?) because it's telling us that things are in an unexpected state instead of just "magically working".

@jamescdavis
Copy link
Member

screen shot 2018-09-10 at 9 46 45 am
❤️

"test:node": "mocha -r register-ts-node 'ts/tests/**/*.{ts,js}'",
"test:all": "ember try:each",
"prepublishOnly": "yarn tsc --project ts --noEmit false",
"postpublish": "rimraf js"
Copy link
Member

Choose a reason for hiding this comment

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

👍 This will help tremendously with:

This could lead to a pretty frustrating DX if one of us happens to have files sitting around in js/ and doesn't realize it.

@mike-north mike-north self-requested a review September 10, 2018 16:18
@mike-north
Copy link
Contributor

I'll review this and play with it today

@mike-north
Copy link
Contributor

@jamescdavis Since you suggest we use both TSLint and ESLint -- Are we truly in a situation where one of these two tools cannot do the whole job?

I typically use TSLint, and have never been able to get the ember:recommended ESLint ruleset working with TS properly -- if we decide to go in this direction, maybe I'll finally learn what I've been missing in terms of configuration and editor settings

@chriskrycho
Copy link
Member

I think @jamescdavis was noting that it's possible to do that (and that OSF is), but not saying we should do it. My overall take is that we should just use ESLint, including doing whatever work is necessary to make that easy to use, in part because ESLint just seems to be on a better foundation and seems to be much more actively developed and it keeps us better in sync with the rest of the Ember ecosystem's defaults… and, frankly, ESLint is a lot less opinionated than TSLint is.

@dfreeman
Copy link
Member Author

dfreeman commented Sep 18, 2018

never been able to get the ember:recommended ESLint ruleset working with TS properly

I suspect the ember:recommended ruleset wouldn't come into play much for our build-time code, though I've also never run into specific issues with it with TS, other than more general problems of e.g. ember/require-super-in-init not applying to ES class definitions.

I do think coming up with a (preferably push-button) recommendation for linting runtime TS code for the community would be a good thing for us to do, but I see that as out of scope here 🙂

I'll also go ahead and vote for 4 space indentation, but that's not a hill I'm willing to die on

I know that's the norm in TS, and honestly it drives me batty 😉. I'd lean toward 2-space, since that's the norm for JS in the Ember community (as well as TS in Ember itself), but also not a hill I'm gonna die on.

Maybe ts_src and js_out?

Entirely possible this is just me, but I think I already assume when I see ts and js that the former is the source and the latter is the output.

I'd love to somehow convey that this is the source for portions of the addon that execute as part of the host's build rather than the runtime stuff that gets shipped to the browser (which would be in addon/ or src/ and in the case of e-c-ts is nothing at all), but haven't been able to come up with any nice way of encoding that in the names.


(...I accidentally hit 'comment and close' instead of focusing the text input to keep typing)

Has anyone had a chance to actually take this branch out for a spin yet? I'm happy to spend time discussing stuff like linting and naming, but I'd also love to get feedback on the practical DX and make sure that this seems sane when it's actually in use 🙂

@dfreeman dfreeman closed this Sep 18, 2018
@dfreeman dfreeman reopened this Sep 18, 2018
@chriskrycho
Copy link
Member

3 space indents with a 90-character maximum!

:ahem:

Yes, that is my extremely unusual personal preference, but no I would never fight someone for it. 😆

re: 4 spaces—that's mostly because TS started very much in a C♯-and-Java-adjacent space. My opinion is that idiomatic TS should be written like idiomatic JS, not like C♯ or Java. Also not a hill I'd die on, but there you have it.


Re: ts_src etc. – I'd be inclined to just do bin/ts and bin/js or similar – that captures what you're getting at, I think, @dfreeman?

@dfreeman
Copy link
Member Author

I think I'd expect anything in bin to be executable? For instance if you look at ember-cli, the only thing in the bin directory there is some bootstrapping code with a shebang.

@chriskrycho
Copy link
Member

Yeah, fair. 🤔

@dfreeman dfreeman force-pushed the build-code-typescript-setup branch from aaadd25 to 9cbb681 Compare September 18, 2018 17:24
@dfreeman
Copy link
Member Author

dfreeman commented Sep 19, 2018

In the same vein, lib/ts + lib/js might be an option, since lib is conventionally where addons put their build-time code. Still a little off, though, as the ts directory in its current form also contains the source for test files. Maybe that's okay, or maybe we could split that out, but it's been nice up to this point managing a single pair of input and output directories.

Copy link
Contributor

@mike-north mike-north left a comment

Choose a reason for hiding this comment

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

Looks great, seems to work well from my quick testing.

RE: the follow-on issues, all reasonable options that have been proposed are fine with me

@dfreeman
Copy link
Member Author

Ok, I'm going to merge this in and start work on getting the Babel integration to a place where folks can begin playing with it.

Here's a summary of where things landed:

  • Linting: eslint is now enabled for .ts files, but I've otherwise left our ruleset alone. Adopting additional conventions here and/or setting more general community-wide defaults can happen as follow on work driven by whoever is interested.
  • Input/output directories: in the absence of a super elegant convention springing fully formed out of the discussion here, I'm sticking with bare ts/js for now so that we're at least consistent with the one other addon doing build-time TypeScript at present. This is easy to change in the future if or when we do come up with something better.
  • Entry point behavior: we still prefer files in js if they're present, but issue a warning if they're being used in development.

@dfreeman dfreeman merged commit 1228ff0 into master Sep 21, 2018
@dfreeman dfreeman deleted the build-code-typescript-setup branch September 21, 2018 19:27
"testdouble": "^3.5.0",
"typescript": "^2.7.2"
"ts-node": "^7.0.1",
"typescript": "^2.7.2",
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion for follow-up: given this is for us, let's update soon to use latest TS here. It fixes quite a few bugs in the type system from what I've seen, and while we're unlikely to hit most of them outside Ember code.

HeroicEric added a commit to HeroicEric/ember-cli-typescript that referenced this pull request Sep 29, 2021
Fixes some links to the `tsconfig.json` blueprint.

These files were moved in typed-ember#288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants