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

Add typescript support #111

Merged
merged 1 commit into from Aug 26, 2019
Merged

Add typescript support #111

merged 1 commit into from Aug 26, 2019

Conversation

ro0gr
Copy link
Contributor

@ro0gr ro0gr commented Mar 13, 2019

I've noticed in the new ember app, import statement for this addon is highlighted with an error in the app.ts. I think it makes sense to provide basic types by default.

I'm wondering which would be the best TS support strategy for a small addon like that, which successfully lived for yers w/o any worries about TS.

Should we:

  • provide .d.ts file and cover with dtslint tests
  • convert the whole lib to TS
  • put d.ts to DefinitelyTyped

@rwjblue
Copy link
Member

rwjblue commented Mar 15, 2019

I'd rather convert to TS than add untested types 🤔

@ro0gr
Copy link
Contributor Author

ro0gr commented Mar 15, 2019

ok, I'll try to do it

@ro0gr
Copy link
Contributor Author

ro0gr commented Mar 15, 2019

would ec-typescript be ok here?

types/loader.js/index.d.ts Outdated Show resolved Hide resolved
addon/index.ts Outdated Show resolved Hide resolved
@ro0gr ro0gr changed the title Add basic typescript definition Add typescript support Mar 15, 2019
@ro0gr
Copy link
Contributor Author

ro0gr commented Jun 17, 2019

@rwjblue are there any issues blocking this PR from being merged? I can fix conflicts if this PR looks good enough to you.

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.

Sorry I missed this PR 😢

Overall, I'm 👍 just left a few small inline comments/tweaks

addon/index.ts Show resolved Hide resolved
addon/index.ts Outdated Show resolved Hide resolved
addon/index.ts Outdated Show resolved Hide resolved
addon/index.ts Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Jun 17, 2019

convert the whole lib to TS

I'd prefer this one

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2019

also needs a rebase

@ro0gr ro0gr force-pushed the types branch 2 times, most recently from f3dc9be to 0c7aeb6 Compare June 19, 2019 20:14
@msalahz
Copy link

msalahz commented Aug 14, 2019

Any update on this?

@ro0gr
Copy link
Contributor Author

ro0gr commented Aug 16, 2019

@msalahz I've been busy a bit with some other activity last months. Going to update this PR today or this weekend.

@ro0gr
Copy link
Contributor Author

ro0gr commented Aug 18, 2019

rebased the branch. There is a test failure for ember-beta scenario

TypeError: Class constructor Promise cannot be invoked without 'new'

tried it locally, but can't reproduce. I think it might be phantom or unrelated to the PR. Maybe restart can help

@ro0gr
Copy link
Contributor Author

ro0gr commented Aug 19, 2019

ya, test failure has to be fixed with ember-source@v3.13.0-beta.3 emberjs/ember-test-helpers#694 (comment)

@ro0gr
Copy link
Contributor Author

ro0gr commented Aug 24, 2019

Rebased again and tests 🍏 . @rwjblue is there something needed to be done to ship this PR?

@rwjblue rwjblue merged commit 8277efa into ember-cli:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants