Skip to content
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

TypeScript ambient type information #44

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mike-north
Copy link

Because this is an 'extension of ember' kind of addon (often used without being explicitly imported), consumers will need to do one of two things in order to take advantage of this type information.

A. Indicate that this type information is to be included

tsconfig.json
{
  "compilerOptions": {
      "types": [
         "ember-cli-head"
       ]
  }
}

B. Import the entry point for this addon from somewhere in their project

app/app.ts
import Application from '@ember/application';
import loadInitializers from 'ember-load-initializers';
import config from './config/environment';
import Resolver from './resolver';
// ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ ⬇️ 
import 'ember-cli-head';

const App = Application.extend({ ... });

After doing either (A) or (B), type-checking will be happy with developers using the headData service implicitly injected onto all routes, without any further ceremony required.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Seems better to me to use headData: inject.service() instead of relying on the injection into all routes (the same is true of things like ember-data’s store service).

In this particular case, I’m unsure how we can deprecate the auto-injection 🤔. Perhaps by adding a getter with deprecation to Ember.Route.prototype...

@mike-north
Copy link
Author

mike-north commented Dec 31, 2017

@rwjblue I think we need to separate what we know to be general best practices

use headData: inject.service() instead of relying on the injection into all routes

from the task of creating type definitions that accurately reflect what the code is doing. In this case, I got a bit turned around (there is no auto-injection), but for cases like these

https://github.com/emberjs/data/blob/0675ef352278a9a489337d2cf46700a768fe8e42/addon/setup-container.js#L72-L74
https://github.com/ronco/ember-cli-meta-tags/blob/299d6c97c0a2a3e7f3f4539ad8d1fa3d6582ed9d/app/initializers/head-tags.js#L4

we should reflect injection onto prototype correctly in type information. I'll update this PR to accurately reflect that explicit injection of the headData service is required.

It's still useful to have this small amount of remaining type information, because of the way ember-cli-meta-tags extends the headData service

@josemarluedke
Copy link
Contributor

Is there any intension in moving TypeScript types here further?

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.

None yet

3 participants