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.

Sign up for GitHub

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

Jump to bottom

Add TypeScript ambient type information #66

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

Add TypeScript ambient type information #66

wants to merge 2 commits into from

Conversation

mike-north
Copy link
Contributor

Fixes #65

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-document-title"
       ]
  }
}

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-document-title';

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

kimroen
kimroen requested changes Jan 21, 2018
View reviewed changes
Copy link
Owner

@kimroen kimroen left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for taking the time to submit this. I appreciate it.

I don't use TypeScript so I can't really comment on this beyond what I can guess from reading the code, but I did have a question.

index.d.ts Outdated
namespace Ember {
interface Route {
titleToken: string;
title: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Is this right? Both titleToken and title can be either a string or a function that can return a few different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type information does need to change to accommodate this. Does the function always return a string?

Copy link
Owner

Choose a reason for hiding this comment

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

title can be

  • A string
  • A function that returns a string

titleToken can be

  • A string
  • A function that returns
    • A string
    • An array of strings
    • And new in the yet to be released version:
      • A promise that resolves to a string
      • An array of promises that resolves to a string

@mike-north
Copy link
Contributor Author

@kimroen I responded to your feedback. This should be good to go now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kimroen kimroen kimroen requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeScript ambient type information
2 participants
@mike-north @kimroen