Skip to content

[WIP] convert to typescript#167

Closed
dwickern wants to merge 1 commit intomasterfrom
typescript
Closed

[WIP] convert to typescript#167
dwickern wants to merge 1 commit intomasterfrom
typescript

Conversation

@dwickern
Copy link
Copy Markdown
Contributor

This is a first pass at converting the addon to typescript. I've converted the addon code, blueprints and supporting code. I haven't touched tests yet.

Most of the effort so far was spent writing type definitions for various ember-cli/broccoli dependencies.

@chriskrycho
Copy link
Copy Markdown
Member

❤️ I'll try to take a gander if you want… but probably after EmberConf.

"ember-cli-release": "^0.2.9",
"ember-cli-shims": "^1.2.0",
"ember-cli-sri": "^2.1.0",
"ember-cli-typescript": "^1.1.6",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe npm would error on this (same name as package) ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't checked on publishing, but certainly none of the local operations complain. You'd never be able to publish any package which wanted to use itself otherwise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, for example typescript is written in typescript and thus has itself as a dependency. Inception!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@dwickern not really, they do not add it in package.json, I believe they use a last known good version here https://github.com/Microsoft/TypeScript/tree/master/lib

And yes that's what I meant @chriskrycho, it won't publish if same name is inside dependencies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Okay this will not cause any problem. Releasing will work too as it is in devDependencies. I just checked with a dummy package :)

Sorry for troubling :D

"test": "ember try:each",
"nodetest": "mocha node-tests --recursive"
"nodetest": "mocha node-tests --recursive",
"prepublishOnly": "ember ts:precompile",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hadn't even occurred to me that this would work for our purposes here—nice! 😄

@dfreeman
Copy link
Copy Markdown
Contributor

What does development/testing of e-c-ts itself look like with this setup? Would we use ts-node or something like that?

@dwickern
Copy link
Copy Markdown
Contributor Author

Personally I've developed using tsc --watch with **/*.js gitignored, and from there it's the same as using regular JS. The next step would be moving to a dist/ directory that we also use for publishing. I haven't looked into ts-node yet.

I had to disable allowSyntheticDefaultImports, otherwise the type checker wouldn't catch incorrect imports. I think ultimately the build and the app will need separate tsconfigs.

@dfreeman
Copy link
Copy Markdown
Contributor

I think ultimately the build and the app will need separate tsconfigs.

Yep, that makes sense. It would be great once we've got this all figured out if we could make it as easy as possible for other addons to use TS in their node-side code.

Until MU lands, there would be some slight weirdness with declaration files (e.g. does index.d.ts refer to the build-time index or the runtime one?), but it still seems like a win overall—especially if we're able to get some of the library types you've written here upstreamed!

@dwickern
Copy link
Copy Markdown
Contributor Author

The root index.d.ts should have typings for index.js, ie. build-time declarations.

@chriskrycho
Copy link
Copy Markdown
Member

chriskrycho commented Mar 19, 2018

@dwickern the problem is that it's overloaded: it also ends up being the root import for when you import something from an addon.

Maybe we should recommend doing ember-cli-build.js and ember-cli-build.d.ts for TS-based addons, for this reason?

@dwickern
Copy link
Copy Markdown
Contributor Author

Isn't that the build for the dummy app? 🙈

@chriskrycho
Copy link
Copy Markdown
Member

@dwickern thank you for starting this so long ago! ❤️ @dfreeman ended up carrying it forward during the push for 2.0, so I'm closing it now.

@chriskrycho chriskrycho deleted the typescript branch April 10, 2019 23:45
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